-
Bug
-
Resolution: Unresolved
-
P4
-
11, 14, 15
A DESCRIPTION OF THE PROBLEM :
The Iterators of the entrySet() and values() returned by java.util.concurrent.ConcurrentHashMap and java.util.concurrent.ConcurrentSkipListMap (and likely sub maps of them as well) behave incorrectly when their remove() method is called but the entry in the map has been modified in the meantime.
For example:
```
ConcurrentMap<String, String> map = new ConcurrentHashMap();
map.put("a", "A");
Iterator<Entry<String, String>> iterator = map.entrySet().iterator();
Entry<String, String> next = iterator.next();
// Replace mapping
map.put("a", "B");
// Should have no effect because Entry("a", "A") does not exist anymore
iterator.remove();
if (map.size() != 1) { // Fails
throw new AssertionError("Wrong map size");
}
```
This is very unintuitive because the documentation does not mention that, instead it even hints that this behavior is a bug (emphasis mine):
> The set supports element removal, which removes the **corresponding mapping** from the map
https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/util/concurrent/ConcurrentHashMap.html#entrySet()
https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/util/concurrent/ConcurrentSkipListMap.html#entrySet()
> The collection supports element removal, which removes the **corresponding mapping** from this map
https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/util/concurrent/ConcurrentHashMap.html#values()
https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/util/concurrent/ConcurrentSkipListMap.html#values()
They both talk about "mapping", and a mapping consists of both key **and value**. However currently the implementation ignores the value.
While one could argue that these collections and their iterators always operate on the current state of the map and therefore this behavior is intended, I would counter that this breaks the contract of Iterator.remove() which says (emphasis mine):
> Removes from the underlying collection the **last element** returned by this iterator
https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/util/Iterator.html#remove()
Regardless of whether this behavior is deemed intended or not, could the documentation please be clarified? The information about weakly consistent iterators (https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/util/concurrent/package-summary.html#Weakly) does not clarify it either.
---------- BEGIN SOURCE ----------
import java.util.Arrays;
import java.util.Iterator;
import java.util.List;
import java.util.Map.Entry;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.ConcurrentSkipListMap;
import java.util.function.Supplier;
public class ConcurrentMapTest {
private static void testEntrySet(Supplier<ConcurrentMap<String, String>> mapFactory) {
ConcurrentMap<String, String> map = mapFactory.get();
map.put("a", "A");
Iterator<Entry<String, String>> iterator = map.entrySet().iterator();
Entry<String, String> next = iterator.next();
assert next.getKey().equals("a") && next.getValue().equals("A");
// Replace mapping
map.put("a", "B");
// Should have no effect because Entry("a", "A") does not exist anymore
iterator.remove();
if (map.size() != 1) { // Fails
throw new AssertionError("Wrong map size");
}
}
private static void testValues(Supplier<ConcurrentMap<String, String>> mapFactory) {
ConcurrentMap<String, String> map = mapFactory.get();
map.put("a", "A");
Iterator<String> iterator = map.values().iterator();
String next = iterator.next();
assert next.equals("A");
// Replace mapping
map.put("a", "B");
// Should have no effect because value "A" does not exist anymore
iterator.remove();
if (map.size() != 1) { // Fails
throw new AssertionError("Wrong map size");
}
}
public static void main(String[] args) {
List<Runnable> tests = Arrays.asList(
() -> testEntrySet(ConcurrentHashMap::new),
() -> testValues(ConcurrentHashMap::new),
() -> testEntrySet(ConcurrentSkipListMap::new),
() -> testValues(ConcurrentSkipListMap::new)
);
tests.forEach(test -> {
try {
test.run();
} catch (AssertionError error) {
error.printStackTrace();
}
});
}
}
---------- END SOURCE ----------
The Iterators of the entrySet() and values() returned by java.util.concurrent.ConcurrentHashMap and java.util.concurrent.ConcurrentSkipListMap (and likely sub maps of them as well) behave incorrectly when their remove() method is called but the entry in the map has been modified in the meantime.
For example:
```
ConcurrentMap<String, String> map = new ConcurrentHashMap();
map.put("a", "A");
Iterator<Entry<String, String>> iterator = map.entrySet().iterator();
Entry<String, String> next = iterator.next();
// Replace mapping
map.put("a", "B");
// Should have no effect because Entry("a", "A") does not exist anymore
iterator.remove();
if (map.size() != 1) { // Fails
throw new AssertionError("Wrong map size");
}
```
This is very unintuitive because the documentation does not mention that, instead it even hints that this behavior is a bug (emphasis mine):
> The set supports element removal, which removes the **corresponding mapping** from the map
https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/util/concurrent/ConcurrentHashMap.html#entrySet()
https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/util/concurrent/ConcurrentSkipListMap.html#entrySet()
> The collection supports element removal, which removes the **corresponding mapping** from this map
https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/util/concurrent/ConcurrentHashMap.html#values()
https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/util/concurrent/ConcurrentSkipListMap.html#values()
They both talk about "mapping", and a mapping consists of both key **and value**. However currently the implementation ignores the value.
While one could argue that these collections and their iterators always operate on the current state of the map and therefore this behavior is intended, I would counter that this breaks the contract of Iterator.remove() which says (emphasis mine):
> Removes from the underlying collection the **last element** returned by this iterator
https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/util/Iterator.html#remove()
Regardless of whether this behavior is deemed intended or not, could the documentation please be clarified? The information about weakly consistent iterators (https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/util/concurrent/package-summary.html#Weakly) does not clarify it either.
---------- BEGIN SOURCE ----------
import java.util.Arrays;
import java.util.Iterator;
import java.util.List;
import java.util.Map.Entry;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.ConcurrentSkipListMap;
import java.util.function.Supplier;
public class ConcurrentMapTest {
private static void testEntrySet(Supplier<ConcurrentMap<String, String>> mapFactory) {
ConcurrentMap<String, String> map = mapFactory.get();
map.put("a", "A");
Iterator<Entry<String, String>> iterator = map.entrySet().iterator();
Entry<String, String> next = iterator.next();
assert next.getKey().equals("a") && next.getValue().equals("A");
// Replace mapping
map.put("a", "B");
// Should have no effect because Entry("a", "A") does not exist anymore
iterator.remove();
if (map.size() != 1) { // Fails
throw new AssertionError("Wrong map size");
}
}
private static void testValues(Supplier<ConcurrentMap<String, String>> mapFactory) {
ConcurrentMap<String, String> map = mapFactory.get();
map.put("a", "A");
Iterator<String> iterator = map.values().iterator();
String next = iterator.next();
assert next.equals("A");
// Replace mapping
map.put("a", "B");
// Should have no effect because value "A" does not exist anymore
iterator.remove();
if (map.size() != 1) { // Fails
throw new AssertionError("Wrong map size");
}
}
public static void main(String[] args) {
List<Runnable> tests = Arrays.asList(
() -> testEntrySet(ConcurrentHashMap::new),
() -> testValues(ConcurrentHashMap::new),
() -> testEntrySet(ConcurrentSkipListMap::new),
() -> testValues(ConcurrentSkipListMap::new)
);
tests.forEach(test -> {
try {
test.run();
} catch (AssertionError error) {
error.printStackTrace();
}
});
}
}
---------- END SOURCE ----------