oracle/coherence

OldCache violates the Map contract

Closed this issue · 3 comments

ben-manes commented

When testing for #16 (comment), I used Guava's testlib to validate conformance to the Collections Framework contracts. Running these tests against the existing OldCache uncovered a bug in the iterator.

CollectionIteratorTester.testIterator_unknownOrderRemoveSupported 
junit.framework.AssertionFailedError: failed with stimuli [hasNext, hasNext, next, hasNext, remove]

	at com.google.common.collect.testing.Helpers.fail(Helpers.java:248)
	at com.google.common.collect.testing.AbstractIteratorTester.compareResultsForThisListOfStimuli(AbstractIteratorTester.java:370)
	at com.google.common.collect.testing.AbstractIteratorTester.recurse(AbstractIteratorTester.java:344)
	at com.google.common.collect.testing.AbstractIteratorTester.recurse(AbstractIteratorTester.java:349)
	at com.google.common.collect.testing.AbstractIteratorTester.recurse(AbstractIteratorTester.java:349)
	at com.google.common.collect.testing.AbstractIteratorTester.recurse(AbstractIteratorTester.java:349)
	at com.google.common.collect.testing.AbstractIteratorTester.recurse(AbstractIteratorTester.java:349)
	at com.google.common.collect.testing.AbstractIteratorTester.recurse(AbstractIteratorTester.java:349)
	at com.google.common.collect.testing.AbstractIteratorTester.test(AbstractIteratorTester.java:317)
	at com.google.common.collect.testing.testers.CollectionIteratorTester.runIteratorTest(CollectionIteratorTester.java:133)
	at com.google.common.collect.testing.testers.CollectionIteratorTester.testIterator_unknownOrderRemoveSupported(CollectionIteratorTester.java:111)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at junit.framework.TestCase.runTest(TestCase.java:177)
	at junit.framework.TestCase.runBare(TestCase.java:142)
	at junit.framework.TestResult$1.protect(TestResult.java:122)
	at junit.framework.TestResult.runProtected(TestResult.java:142)
	at junit.framework.TestResult.run(TestResult.java:125)
	at junit.framework.TestCase.run(TestCase.java:130)
	at junit.framework.TestSuite.runTest(TestSuite.java:241)
	at junit.framework.TestSuite.run(TestSuite.java:236)
	at junit.framework.TestSuite.runTest(TestSuite.java:241)
	at junit.framework.TestSuite.run(TestSuite.java:236)
	at junit.framework.TestSuite.runTest(TestSuite.java:241)
	at junit.framework.TestSuite.run(TestSuite.java:236)
	at junit.framework.TestSuite.runTest(TestSuite.java:241)
	at junit.framework.TestSuite.run(TestSuite.java:236)
	at junit.framework.TestSuite.runTest(TestSuite.java:241)
	at junit.framework.TestSuite.run(TestSuite.java:236)
	at org.junit.internal.runners.JUnit38ClassRunner.run(JUnit38ClassRunner.java:90)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:69)
	at com.intellij.rt.junit.IdeaTestRunner$Repeater$1.execute(IdeaTestRunner.java:38)
	at com.intellij.rt.execution.junit.TestsRepeater.repeat(TestsRepeater.java:11)
	at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:35)
	at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:235)
	at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:54)
Caused by: junit.framework.AssertionFailedError: Target threw exception when reference did not
	at com.google.common.collect.testing.Helpers.fail(Helpers.java:248)
	at com.google.common.collect.testing.AbstractIteratorTester.internalExecuteAndCompare(AbstractIteratorTester.java:442)
	at com.google.common.collect.testing.AbstractIteratorTester.access$600(AbstractIteratorTester.java:44)
	at com.google.common.collect.testing.AbstractIteratorTester$8.executeAndCompare(AbstractIteratorTester.java:553)
	at com.google.common.collect.testing.AbstractIteratorTester.compareResultsForThisListOfStimuli(AbstractIteratorTester.java:367)
	... 37 more
Caused by: java.lang.IllegalStateException
	at com.tangosol.util.FilterEnumerator.remove(FilterEnumerator.java:157)
	at com.google.common.collect.testing.AbstractIteratorTester$1.execute(AbstractIteratorTester.java:469)
	at com.google.common.collect.testing.AbstractIteratorTester.internalExecuteAndCompare(AbstractIteratorTester.java:398)
	... 40 more

There were also failures due to the custom string representation. Unfortunately there is not a setting to ignore that, as it does seem reasonable for your use-case when debugging. Source code below.

OldCacheMapTests 
import com.google.common.collect.testing.MapTestSuiteBuilder;
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 com.tangosol.net.cache.OldCache;
import java.util.Map;
import java.util.Map.Entry;
import junit.framework.TestCase;
import junit.framework.TestSuite;

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

  public static TestSuite suite() {
    return MapTestSuiteBuilder
        .using(generator())
        .named("OldCache")
        .withFeatures(
            CollectionSize.ANY,
            MapFeature.GENERAL_PURPOSE,
            MapFeature.ALLOWS_NULL_KEYS,
            MapFeature.ALLOWS_NULL_VALUES,
            CollectionFeature.SUPPORTS_ITERATOR_REMOVE)
        .createTestSuite();
  }

  private static TestStringMapGenerator generator() {
    return new TestStringMapGenerator() {
      @Override protected Map<String, String> create(Entry<String, String>[] entries) {
        var cache = new OldCache();
        for (var entry : entries) {
          cache.put(entry.getKey(), entry.getValue());
        }
        return cache;
      }
    };
  }
}
aseovic commented

Other than documenting difference in behavior, I'm not sure we can do much about this.

The issue is that the call to hasNext advances underlying iterator and pre-fetches the value to be returned by the subsequent call to next, in order to filter out expired entries. In the process, it explicitly disables remove and causes it to throw IllegalStateException, because at that point underlying iterator is already at the next value, and calling remove would remove wrong value.

It's not ideal, but it's only an issue if you call hasNext between the calls to next and remove, so in most typical usage scenarios it shouldn't be a problem.

ben-manes commented

fwiw, I only just realized you can suppress individual tests. You might consider incorporating their suite as a freebie, but disabling those two that don't fit. That is what fastutils did for their usage.

Caffeine has the same need to peek ahead and passes this test case. Maybe it's iterator implementation would be a helpful reference.

ben-manes commented

To disable some of the Guava tests see FastUtil's setup. This uses suppressing(Method method).

/**
 * Prevents the given methods from being run as part of the test suite.
 *
 * <p><em>Note:</em> in principle this should never need to be used, but it might be useful if the
 * semantics of an implementation disagree in unforeseen ways with the semantics expected by a
 * test, or to keep dependent builds clean in spite of an erroneous test.
 */
public B suppressing(Method... methods)