Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8254053

ConcurrentHashMap and ConcurrentSkipListMap entrySet() and values() remove

XMLWordPrintable

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

            dl Doug Lea
            webbuggrp Webbug Group
            Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

              Created:
              Updated: