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?

The `java.util.List.get(int value)` expects a **int** value which is the **index** of the String element you want to retrieve, in your case your are supplying a `String` to `get()` which cannot compile. Also what are you trying to achieve please elaborate? — Fullstack Guy, Aug 06 '19 at 16:50
@AmardeepBhowmick, based on some condition, critical section has to behave as is the requirement — Sriram M, Aug 06 '19 at 17:01
synchronized() expects an object. `values.get(value) == null ? value : values.get(value)` is an expression which evaluates to an object. So when executing that code, it will synchronize on the object that is the result of the evaluation of the expression. There is nothing different between an expression passed to synchronized and any other Java expression. Synchronizing on Strings, and even worse, String literals or String passed as argument, is a recipe for disaster. — JB Nizet, Aug 06 '19 at 17:03
@JBNizet is right, and it gets even worse with string [interning](https://stackoverflow.com/questions/3052442/what-is-the-difference-between-text-and-new-stringtext) — Avi, Aug 06 '19 at 17:05
@Avi Initially it was a `List` later OP changed it to a `Map`. — Fullstack Guy, Aug 06 '19 at 17:08
Could be an _[XY Problem](https://meta.stackexchange.com/questions/66377/what-is-the-xy-problem)_. Maybe you could explain _why_ you want your code to choose the lock at run-time. This looks like maybe you are trying to reinvent [striped locks](https://github.com/google/guava/wiki/StripedExplained). — Solomon Slow, Aug 06 '19 at 17:13
@SolomonSlow,Avi, JB Nizet, Thanks for your response. The requirement is like need an synchronization for set of objects, and those objects will be decided at runtime. At any point of time, only one object should do the critical task and rest has to wait. — Sriram M, Aug 07 '19 at 09:28
@SriramM, It's very common for Java programs to have one lock object for each instance of a class whose members need protection. Usually the lock either is a private `Object` member of the instance, or else it is the instance itself. What you're trying to do--using a key to choose one member of relatively small set of lock objects--is much less common. I was wondering why you think that approach would help. I can't guess that from your example because the example tells us nothing about the data that you want to protect. — Solomon Slow, Aug 07 '19 at 10:52
@SolomonSlow, ok. Let me explain the requirement in detail, I have a map and the complexObject will get updated from an HTTP request based on the string value. Assume { ("a", obj1), ("b", obj2), ("c", obj3) }. When 2 HTTP request made for the same string value lets say "a", 2 thread tries to do update on the map for obj1. so, it should be synchronized. At the same time, if any request come for different string value (e.g "b"), map should be freely available to update by other thread irrespective of the lock. — Sriram M, Aug 07 '19 at 13:33
@JBNizet …besides accessing a `HashMap` *before* entering the `synchronized` block. — Holger, Aug 19 '19 at 10:47

score 2 · Accepted Answer · answered Aug 21 '19 at 17:15

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.

synchronized block has a logic

1 Answers1