synchronized block has a logic

Question

There is special need for creating thread monitor based on the string value.

Ex:

    Map<String, String> values = new HashMap<>(); (instance variable)
    values.put("1", "one");values.put("2", "two");values.put("3", "three");


    void someMethod(String value) {
      synchronized(values.get(value) == null ? value : values.get(value)) {
        sout("I'm done");
      }
    }

The catch here is synchronized block has a ternary operator, is it allowed? I don't get any compile/run time exception or error.

I'm not sure about the above code really thread safe, at a time only one thread has to obtain the system monitor based on the string value.

Please provide thoughts on this. is this good practice or any other way around?

Answer 1

There are fundamental problems with this approach. You’re accessing a HashMap, which is not thread safe, before ever entering the synchronized block. If there are updates to the map after its construction, this approach is broken.

It’s crucial to use the same object instance for synchronizing when accessing the same data.

So even if you used a thread safe map here, using values.get(value) == null? value: values.get(value) means using changing objects for synchronization, when there are map updates, sometimes it uses the key, sometimes the mapped value, depending on whether a mapping is present. Even when the key is always present, it may use different mapped values.

It’s also pertinent to the Check-Then-Act anti-pattern, as you are checking values.get(value) == null first, and using values.get(value) afterwards, when the condition could have changed already.

You should never use strings for synchronization, as different string objects may be equal, so they map to the same data when using them as key to a Map, whereas synchronization fails due to different object identity. On the other hand, strings may get shared freely in a JVM and they are in case of string literals, so unrelated code performing synchronization on strings could block each other.

There’s a simple solution using a tool designed for this purpose. When using

ConcurrentMap<String, String> values = new ConcurrentHashMap<>();

void someMethod(String string) {
    values.compute(string, (key,value) -> {
        if(value == null) value = key.toUpperCase(); // construct when not present
        // update value
        return value;
    });
}

the string’s equality determines the mutual exclusion while not serving as the synchronization key itself. So equal keys provide the desired blocking, while unrelated code, e.g. using a different ConcurrentHashMap with similar or even the same key values, is not affected by these operations.