Skip to content

Commit cd239d3

Browse files
committed
Use same key match for containsKey
The containsKey method was never modified to support identity mode and will inccorrectly return true for distinct object keys that are equals and have the same identity hashcode. This led to bugs in Marshal.dump when many of the same value occurred in an object graph. containsKey would report that such an object was already in the link map, but a subsequent get of that key would produce -1 to be written to the dump. Later loading of that dump would error due to the invalid link index. The fix here modifies containsKey to use the same matching logic as get, with awareness of identity comparison. A test is included that will usually exercise the given logic, if it can produce a duplicate key object within a 5s timeout. If it cannot do that, it will no-op and skip the test. On my MacBook Air M4 with 10ish cores, it finds such a duplicate about 90% of the time. Fixes jruby#9152
1 parent c6e6679 commit cd239d3

2 files changed

Lines changed: 57 additions & 6 deletions

File tree

core/src/main/java/org/jruby/util/collections/HashMapInt.java

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -95,12 +95,12 @@ public boolean containsValue(int value) {
9595
return contains(value);
9696
}
9797

98-
public boolean containsKey(Object key) {
98+
public boolean containsKey(V key) {
9999
Entry<V>[] tab = table;
100100
int hash = getHash(key);
101101
int index = (hash & 0x7FFFFFFF) % tab.length;
102102
for (Entry<V> e = tab[index]; e != null; e = e.next) {
103-
if (e.hash == hash && e.key.equals(key)) {
103+
if (e.hash == hash && keyMatches(key, e)) {
104104
return true;
105105
}
106106
}
@@ -307,10 +307,7 @@ public int size() {
307307

308308
@Override
309309
public boolean contains(Object o) {
310-
if(o instanceof Number) {
311-
return containsKey(((Number)o).intValue());
312-
}
313-
return false;
310+
return containsKey((V) o);
314311
}
315312

316313
@Override
Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
package org.jruby.util.collections;
2+
3+
import org.junit.Test;
4+
5+
import static java.lang.Runtime.getRuntime;
6+
import static java.util.concurrent.CompletableFuture.anyOf;
7+
import static java.util.concurrent.TimeUnit.SECONDS;
8+
import static org.junit.Assert.*;
9+
10+
import java.util.Arrays;
11+
import java.util.concurrent.CompletableFuture;
12+
import java.util.concurrent.ExecutionException;
13+
14+
/**
15+
* Test for {@link HashMapInt}
16+
*/
17+
public class HashMapIntTest {
18+
19+
@Test
20+
public void testContainsKey() throws Throwable {
21+
HashMapInt<String> map = new HashMapInt<>();
22+
map.put("present", 1);
23+
24+
assertTrue("Should contain 'present'", map.containsKey("present"));
25+
assertFalse("Should not contain 'absent'", map.containsKey("absent"));
26+
27+
// with identity mode
28+
map = new HashMapInt<>(1, true);
29+
30+
String key1 = new String();
31+
map.put(key1, 1);
32+
33+
// Generate a duplicate key concurrently
34+
CompletableFuture[] list = new CompletableFuture[getRuntime().availableProcessors() * 2];
35+
Arrays.setAll(list, i -> CompletableFuture.supplyAsync(() -> findDuplicate(key1)).orTimeout(5, SECONDS));
36+
String key2;
37+
try {
38+
key2 = (String) anyOf(list).get();
39+
} catch (ExecutionException e) {
40+
// could not find a duplicate key within timeout, skip test
41+
return;
42+
}
43+
44+
assertTrue("Should contain key1", map.containsKey(key1));
45+
assertFalse("Should not contain duplicate but idempotent key2", map.containsKey(key2));
46+
}
47+
48+
private static Object findDuplicate(Object key1) {
49+
while (true) {
50+
String key = new String();
51+
if (System.identityHashCode(key1) == System.identityHashCode(key)) return key;
52+
}
53+
}
54+
}

0 commit comments

Comments
 (0)