Repository: real-logic/agrona
Issue: ObjectHashSet does not implement equals in a compatible fashion to HashSet/AbstractSet

In essence, it does not accommodate other implementations of the Set interface, such that a HashSet and an ObjectHashSet of the same contents are not equal. This blocks the adoption of ObjectHashSet as a drop in replacement. Happy to provide a PR if we agree this is desirable to be fixed.

Changed files:
agrona/src/main/java/org/agrona/collections/ObjectHashSet.java, agrona/src/test/java/org/agrona/collections/IntHashSetTest.java, agrona/src/main/java/org/agrona/collections/IntHashSet.java, agrona/src/test/java/org/agrona/collections/ObjectHashSetIntegerTest.java

diff --git a/agrona/src/main/java/org/agrona/collections/IntHashSet.java b/agrona/src/main/java/org/agrona/collections/IntHashSet.java
index c29d0af1..552dc4be 100644
--- a/agrona/src/main/java/org/agrona/collections/IntHashSet.java
+++ b/agrona/src/main/java/org/agrona/collections/IntHashSet.java
@@ -652,7 +652,29 @@ public final class IntHashSet extends AbstractSet<Integer> implements Serializab
                    containsAll(otherSet);
         }
 
-        return false;
+        if (!(other instanceof Set))
+        {
+            return false;
+        }
+
+        final Set<?> c = (Set<?>)other;
+        if (c.size() != size())
+        {
+            return false;
+        }
+
+        try
+        {
+            return containsAll(c);
+        }
+        catch (final ClassCastException unused)
+        {
+            return false;
+        }
+        catch (final @DoNotSub NullPointerException unused)
+        {
+            return false;
+        }
     }
 
     /**
diff --git a/agrona/src/main/java/org/agrona/collections/ObjectHashSet.java b/agrona/src/main/java/org/agrona/collections/ObjectHashSet.java
index 5917d007..26654871 100644
--- a/agrona/src/main/java/org/agrona/collections/ObjectHashSet.java
+++ b/agrona/src/main/java/org/agrona/collections/ObjectHashSet.java
@@ -566,7 +566,29 @@ public final class ObjectHashSet<T> extends AbstractSet<T> implements Serializab
             return otherSet.size == size && containsAll(otherSet);
         }
 
-        return false;
+        if (!(other instanceof Set))
+        {
+            return false;
+        }
+
+        final Set<?> c = (Set<?>)other;
+        if (c.size() != size())
+        {
+            return false;
+        }
+
+        try
+        {
+            return containsAll(c);
+        }
+        catch (final ClassCastException unused)
+        {
+            return false;
+        }
+        catch (final NullPointerException unused)
+        {
+            return false;
+        }
     }
 
     /**
diff --git a/agrona/src/test/java/org/agrona/collections/IntHashSetTest.java b/agrona/src/test/java/org/agrona/collections/IntHashSetTest.java
index 4ac0d2ea..23ecadfc 100644
--- a/agrona/src/test/java/org/agrona/collections/IntHashSetTest.java
+++ b/agrona/src/test/java/org/agrona/collections/IntHashSetTest.java
@@ -626,63 +626,6 @@ public class IntHashSetTest
         assertThat(testSet, hasSize(1));
     }
 
-    private static void addTwoElements(final IntHashSet obj)
-    {
-        obj.add(1);
-        obj.add(1001);
-    }
-
-    private static void addTwoElements(final HashSet<Integer> obj)
-    {
-        obj.add(1);
-        obj.add(1001);
-    }
-
-    private void assertIteratorHasElements()
-    {
-        final Iterator<Integer> iter = testSet.iterator();
-
-        final Set<Integer> values = new HashSet<>();
-
-        assertTrue(iter.hasNext());
-        values.add(iter.next());
-        assertTrue(iter.hasNext());
-        values.add(iter.next());
-        assertFalse(iter.hasNext());
-
-        assertContainsElements(values);
-    }
-
-    private void assertIteratorHasElementsWithoutHasNext()
-    {
-        final Iterator<Integer> iter = testSet.iterator();
-
-        final Set<Integer> values = new HashSet<>();
-
-        values.add(iter.next());
-        values.add(iter.next());
-
-        assertContainsElements(values);
-    }
-
-    private static void assertArrayContainingElements(final Integer[] result)
-    {
-        assertThat(result, arrayContainingInAnyOrder(1, 1001));
-    }
-
-    private static void assertContainsElements(final Set<Integer> other)
-    {
-        assertThat(other, containsInAnyOrder(1, 1001));
-    }
-
-    private void exhaustIterator()
-    {
-        final IntIterator iterator = testSet.iterator();
-        iterator.next();
-        iterator.next();
-        iterator.next();
-    }
-
     @Test
     public void shouldNotContainMissingValueInitially()
     {
@@ -861,4 +804,84 @@ public class IntHashSetTest
 
         assertFalse(testSet.contains(MISSING_VALUE));
     }
+
+    @Test
+    public void shouldHaveCompatibleEqualsAndHashcode()
+    {
+        final HashSet compatibleSet = new HashSet();
+        final long seed = System.nanoTime();
+        final Random r = new Random(seed);
+        for (int i = 0; i < 1024; i++)
+        {
+            final int value = r.nextInt();
+            compatibleSet.add(value);
+            testSet.add(value);
+        }
+
+        if (r.nextBoolean())
+        {
+            compatibleSet.add(MISSING_VALUE);
+            testSet.add(MISSING_VALUE);
+        }
+        assertEquals("Fail with seed:" + seed, testSet, compatibleSet);
+        assertEquals("Fail with seed:" + seed, compatibleSet, testSet);
+        assertEquals("Fail with seed:" + seed, compatibleSet.hashCode(), testSet.hashCode());
+    }
+
+    private static void addTwoElements(final IntHashSet obj)
+    {
+        obj.add(1);
+        obj.add(1001);
+    }
+
+    private static void addTwoElements(final HashSet<Integer> obj)
+    {
+        obj.add(1);
+        obj.add(1001);
+    }
+
+    private void assertIteratorHasElements()
+    {
+        final Iterator<Integer> iter = testSet.iterator();
+
+        final Set<Integer> values = new HashSet<>();
+
+        assertTrue(iter.hasNext());
+        values.add(iter.next());
+        assertTrue(iter.hasNext());
+        values.add(iter.next());
+        assertFalse(iter.hasNext());
+
+        assertContainsElements(values);
+    }
+
+    private void assertIteratorHasElementsWithoutHasNext()
+    {
+        final Iterator<Integer> iter = testSet.iterator();
+
+        final Set<Integer> values = new HashSet<>();
+
+        values.add(iter.next());
+        values.add(iter.next());
+
+        assertContainsElements(values);
+    }
+
+    private static void assertArrayContainingElements(final Integer[] result)
+    {
+        assertThat(result, arrayContainingInAnyOrder(1, 1001));
+    }
+
+    private static void assertContainsElements(final Set<Integer> other)
+    {
+        assertThat(other, containsInAnyOrder(1, 1001));
+    }
+
+    private void exhaustIterator()
+    {
+        final IntIterator iterator = testSet.iterator();
+        iterator.next();
+        iterator.next();
+        iterator.next();
+    }
 }
diff --git a/agrona/src/test/java/org/agrona/collections/ObjectHashSetIntegerTest.java b/agrona/src/test/java/org/agrona/collections/ObjectHashSetIntegerTest.java
index 43ce0a91..acded0a2 100644
--- a/agrona/src/test/java/org/agrona/collections/ObjectHashSetIntegerTest.java
+++ b/agrona/src/test/java/org/agrona/collections/ObjectHashSetIntegerTest.java
@@ -615,6 +615,24 @@ public class ObjectHashSetIntegerTest
         assertFalse(iter.hasNext());
     }
 
+    @Test
+    public void shouldHaveCompatibleEqualsAndHashcode()
+    {
+        final HashSet compatibleSet = new HashSet();
+        final long seed = System.nanoTime();
+        final Random r = new Random(seed);
+        for (int i = 0; i < 1024; i++)
+        {
+            final int value = r.nextInt();
+            compatibleSet.add(value);
+            testSet.add(value);
+        }
+
+        assertEquals("Fail with seed:" + seed, testSet, compatibleSet);
+        assertEquals("Fail with seed:" + seed, compatibleSet, testSet);
+        assertEquals("Fail with seed:" + seed, compatibleSet.hashCode(), testSet.hashCode());
+    }
+
     private static void addTwoElements(final ObjectHashSet<Integer> obj)
     {
         obj.add(1);