Behavior of entrySet().removeIf in ConcurrentHashMap
I also managed to reproduce such case on my machine.I think, the problem is that EntrySetView
(which is returned by ConcurrentHashMap.entrySet()
) inherits its removeIf
implementation from Collection
, and it looks like:
default boolean removeIf(Predicate<? super E> filter) { Objects.requireNonNull(filter); boolean removed = false; final Iterator<E> each = iterator(); while (each.hasNext()) { // `test` returns `true` for some entry if (filter.test(each.next())) { // entry has been just changed, `test` would return `false` now each.remove(); // ...but we still remove removed = true; } } return removed; }
In my humble opinion, this cannot be considered as a correct implementation for ConcurrentHashMap
.
After discussion with user Zielu in comments below Zielu's answer I have gone deeper into the ConcurrentHashMap code and found out that:
- ConcurrentHashMap implementation provides
remove(key, value)
method which callsreplaceNode(key, null, value)
replaceNode
checks if both key and value are still present in the map before removing so using it should be fine. Documentation says that it
Replaces node value with v, conditional upon match of cv if * non-null.
- In the case mentioned in the question ConcurrentHashMap's
.entrySet()
is called which returnsEntrySetView
class. ThenremoveIf
method calls.iterator()
which returnsEntryIterator
. EntryIterator
extendsBaseIterator
and inheritsremove
implementation that callsmap.replaceNode(p.key, null, null)
which disables conditional removal and just always removes the key.
The negative course of events could be still prevented if iterators always iterated over 'current' values and never returned old ones if some value is modified. I still don't know if that happens or not, but the test case mentioned below seems to verify the whole thing.
I think that have created a test case which shows that the behavior described in my question can really happen. Please correct me if I there are any mistakes in the code.
The code starts two threads. One of them (DELETING_THREAD) removes all entries mapped to 'false' boolean value. Another one (ADDING_THREAD) randomly puts (1, true)
or (1,false)
values into the map. If it puts true
in the value it expects that the entry will still be there when checked and throws an exception if it is not. It throws an exception quickly when I run it locally.
package test;import java.util.Random;import java.util.concurrent.ConcurrentHashMap;public class MainClass { private static final Random RANDOM = new Random(); private static final ConcurrentHashMap<Integer, Boolean> MAP = new ConcurrentHashMap<Integer, Boolean>(); private static final Integer KEY = 1; private static final Thread DELETING_THREAD = new Thread() { @Override public void run() { while (true) { MAP.entrySet().removeIf(entry -> entry.getValue() == false); } } }; private static final Thread ADDING_THREAD = new Thread() { @Override public void run() { while (true) { boolean val = RANDOM.nextBoolean(); MAP.put(KEY, val); if (val == true && !MAP.containsKey(KEY)) { throw new RuntimeException("TRUE value was removed"); } } } }; public static void main(String[] args) throws InterruptedException { DELETING_THREAD.setDaemon(true); ADDING_THREAD.start(); DELETING_THREAD.start(); ADDING_THREAD.join(); }}