10

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);
}
Improve this question
12
  • 1
    that would also throw a nullpoint exception because you cant call notify() on null Commented Jul 19, 2018 at 7:39
  • @PhilippSander for the first thread it would work. Because when calling notify, the value has not yet been removed Commented Jul 19, 2018 at 7:41
  • @Lino What if the method called before having the K key It will throw another NullPointerException before entering the if statement Commented Jul 19, 2018 at 7:44
  • 1
    @Tiina that would be a race condition, so you can get different results based on timing. However the original question makes more sense if we assume that threads arrive at the same time (in which case there would be no NPE). Commented Jul 19, 2018 at 8:02
  • 1
    I don’t get your question. The answer to the question you’ve linked, does already tell you that synchronizing on a mutable variable is completely broken, so what do you expect from exchanging the mutable variable with a mutable map, which adds even more possible race conditions? Commented Jul 19, 2018 at 12:13

2 Answers 2

Reset to default
10

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.

Improve this answer
Sign up to request clarification or add additional context in comments.

9 Comments

@Lino Absolutely. Thanks!!
@Tiina No, running v = null; won't affect the other thread at all. v (or val in this answer) is a method-local variable and won't change anything for other threads.
@ernest_k how can you predict what the second thread will have, when both threads execute the first map.get(k) without any synchronization? The evaluation of map.get(k) is part of the what the threads do, not happening “before the first one starts executing”. The second thread may see the value the map had before the removal, but it may also see the null reference or break in an entirely different way due to seeing an inconsistent state of the map, as it doesn’t look like the OP is using a thread safe map.
@Holger That's a valid point, but the question was specific about one thing: what would happen when 2 threads compete the lock for map.get(k). When thread A wins..., which clearly supposes that the lock is attempted on the map value, i.e., beyond the null pointer access. Threads won't "compete" for the lock after one had gotten it and modified the map, the second one would simply crash. So the premise is slightly different.
@ernest_k but you have identified the flaw in that premise yourself right in your answer: “second thread won't ’request’ a lock on thread.get(k), both threads will request a lock on the result of map.get(k)”, but the result of map.get(k) is unpredictable. Your answer does not indicate that the assumption that thread two reads a non-null value stems from taking the OP’s flawed premise for granted, misguiding the OP into thinking that this was a working solution while it is actually broken.
|
3

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".

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.