java concurrency synchronized on map value

Question

The following code, I am confused about what would happen when 2 threads compete the lock for map.get(k). When thread A wins, it makes map.get(k) null and the second thread would get a synchronized(null)? Or would it be both threads see it as synchronized(v) even though the first thread changes it to null but during which thread B still sees it as v?

synchronized(map.get(k)) {
   map.get(k).notify();
   map.remove(k);
}

The question is a similar to another question, except lock object is value of a map.

UPDATE: compared the discussion in this post and that in the above link, is it true that

synchronized(v) {
    v.notify();
    v = null;
} 

would cause the 2nd thread synchronized(null). But for the synchronized(map.get(k)), the 2nd thread would have synchronized(v)???

UPDATE: To answer @Holger's question, the main difference between this post and the other one is:

final V v = new V();
synchonized(map.get(k)) {
    map.get(k).notify();
    map.remove(k);
}

Answer 1

The second thread won't "request" a lock on thread.get(k), both threads will request a lock on the result of map.get(k) before the first one starts executing. So the code is roughly similar to:

Object val = map.get(k);
val.notify();

So, when the thread that obtained the lock finishes executing, the second thread will still have a reference to Object val, even if map[k] doesn't point to it anymore (or points to null)

---

EDIT: (following many useful comments)

It seems that the lock on map.get(k) is being acquired to ensure that the processing is done only once (map.remove(k) is called after processing). While it's true that 2 threads that compete for the lock on val won't run into null.notify(), the safety of this code is not guaranteed as the second thread may call synchronized(map.get(k)) after the first one has exited the synchronized block.

To ensure that the k is processed atomically, a safer approach may be needed. One way to do this is to use a concurrent hash map, like below:

map.computeIfPresent(k, (key, value) -> {
    //process the value here
    //key is k
    //value is the value to which k is mapped.

    return null; //return null to remove the value after processing.
});

Please note that map in the preceding example is an instance of ConcurrentHashMap. This will ensure that the value is processed once (computeIfPresent runs atomically).

To quote ConcurrentHashMap.computeIfAbsent doc comments:

If the value for the specified key is present, attempts to compute a new mapping given the key and its current mapped value. The entire method invocation is performed atomically. Some attempted update operations on this map by other threads may be blocked while computation is in progress, so the computation should be short and simple, and must not attempt to update any other mappings of this map.

Answer 2

What would happen is that you would lock on the value currently in the hashmap entry for key k.

Problem #1 - if the map.get(k) call returns null, then you would get an NPE.

Problem #2 - since you are not locking on map:

• you are likely to get race conditions with other threads; e.g. if some other thread does a map.put(k, v) with a different v to the one you are locking, and
• the map.remove(k) may result in memory anomalies leading (potentially) to corruption of the map data structure.

It is not clear what you are actually trying to achieve by synchronizing on map.get(k) (rather than map). But whatever it is, this code is not thread-safe.

---

Re your update: Yes that is true ... assuming that other thread is synchronizing on the value of the same variable v. Note that you always synchronize on an object, so when you do synchronized(v), that means "take the current value of v and synchroize on that object".