ConcurrentReferenceHashMap's entrySet violates the Map contract

[ben-manes]

A quick sanity test of this map using Guava's testlib found some simple violations. For example entrySet().iterator() does not throw an exception for the sequence [hasNext, hasNext, next, remove, remove]. In this case Iterator.remove() failed to null out the last property after the first call, so a subsequent call does not throw an IllegalStateException. As the keySet and values views delegate to entrySet, this error is found multiple times in Guava's suite. You might consider using this suite on other custom collections.

Unit Tests

import java.util.Map;

import org.springframework.util.ConcurrentReferenceHashMap;

import com.google.common.collect.testing.ConcurrentMapTestSuiteBuilder;
import com.google.common.collect.testing.TestStringMapGenerator;
import com.google.common.collect.testing.features.CollectionFeature;
import com.google.common.collect.testing.features.CollectionSize;
import com.google.common.collect.testing.features.MapFeature;

import junit.framework.Test;
import junit.framework.TestCase;

/** Guava testlib map tests. */
public final class ConcurrentReferenceHashMapTests extends TestCase {

  public static Test suite() {
    var suite = ConcurrentMapTestSuiteBuilder
        .using(new TestStringMapGenerator() {
          @Override protected Map<String, String> create(Map.Entry<String, String>[] entries) {
            var map = new ConcurrentReferenceHashMap<String, String>();
            for (var entry : entries) {
              map.put(entry.getKey(), entry.getValue());
            }
            return map;
          }
        })
        .named("ConcurrentReferenceHashMap")
        .withFeatures(
            MapFeature.GENERAL_PURPOSE,
            MapFeature.ALLOWS_ANY_NULL_QUERIES,
            CollectionFeature.SUPPORTS_ITERATOR_REMOVE,
            CollectionSize.ANY);
    return suite.createTestSuite();
  }
}

[ben-manes]

@jhoeller Thanks for the quick fix. I checked with Guava's tests and it looks like the other failures (like Map.contains) were resolved thanks to this and perhaps other fixes on 6.0-SNAPSHOT (I had only checked the last 5.x release). You might consider adding Guava's testlib dependency (their testing utilities, com.google.guava:guava-testlib) and the above suite to yours, and verify other collections like LinkedCaseInsensitiveMap (passes below test).

public static Test suite() {
  var suite = MapTestSuiteBuilder
      .using(new TestStringMapGenerator() {
        @Override protected Map<String, String> create(Map.Entry<String, String>[] entries) {
          var map = new LinkedCaseInsensitiveMap<String>();
          for (var entry : entries) {
            map.put(entry.getKey(), entry.getValue());
          }
          return map;
        }
      })
      .named("LinkedCaseInsensitiveMap")
      .withFeatures(
          MapFeature.GENERAL_PURPOSE,
          MapFeature.ALLOWS_NULL_VALUES,
          CollectionFeature.SUPPORTS_ITERATOR_REMOVE,
          CollectionSize.ANY);
  return suite.createTestSuite();
}
<details>

[jhoeller]

@ben-manes Thanks for raising this to begin with! Those violations remained completely unnoticed before, in particular the buggy equals comparison in the EntrySet.contains implementation. I suppose that they are simply not part of the regular usage model for that particular collection. I wouldn't be surprised if noone ever called entrySet() on a ConcurrentReferenceHashMap before since that collection is only really used for very specific interaction patterns.

LinkedCaseInsensitiveMap has certainly seen wider-spread use against the actual Map contract, also with the Java 8 additions to the Map interface, with quite a few contract violation bugs reported over the years.

I forgot to comment on guava-testlib yesterday: I locally gave it a try and used it to verify the fix - but also added individual unit tests for those particular changes. I'll raise guava-testlib in our next team call, it would indeed be a fine additional regression testing measure for our test suite, even with it being in old JUnit 3.8 shape.