Let's say I have the following field inside of a class:
ConcurrentHashMap<SomeClass, Set<SomeOtherClass>> myMap = new ConcurrentHashMap<SomeClass, Set<SomeOtherClass>>();
An instance of this class is shared among many threads.
If I want to add or remove an element from a set associated with a key, I could do:
Set<SomeOtherClass> setVal = myMap.get(someKeyValue);
setVal.add(new SomeOtherClass());
The get
operation is atomic, therefore thread-safe. However, there's no guarantee that, in between the get
and the add
instruction, some other thread won't modify the structure messing with the first one's execution.
What would be the best way to make the whole operation atomic?
Here's what I've come up with, but I don't believe it to be very efficient (or to be making the best use out of Java's structures):
I have a ReentrantLock field, so my class looks like this:
class A {
ReentrantLock lock = new ReentrantLock();
ConcurrentHashMap<SomeClass, Set<SomeOtherClass>> myMap = new ConcurrentHashMap<SomeClass, Set<SomeOtherClass>>();
}
Then the method call looks something like this:
lock.lock();
Set<SomeOtherClass> setVal = myMap.get(someKeyValue);
synchronized(setVal) {
lock.unlock();
setVal.add(new SomeOtherClass());
}
The idea being that we let go of the lock once we have made sure no one else will access the Set we're trying to modify. However, I don't think this is making the best use of the ConcurrentMap
or that it makes much sense to have a lock, a concurrent structure, and a synchronized
block all used to achieve one operation.
Is there a better way to go about this?
CodePudding user response:
ConcurrentHashMap
guarantees that the entire method invocation of compute
(or computeIfAbsent
or computeIfPresent
) is done atomically. So, e.g., you could do something like this:
myMap.compute(someKeyValue, (k, v) -> {v.add(new SomeOtherClass()); return v;});
Note:
Using compute
is analogous to the original snippet that assumes that somKeyValue
is present in the map. Using computeIfPresent
is probably safer, though.