Skip to content

Commit 6dd6d79

Browse files
jderegclaude
andcommitted
Fix 5 correctness bugs in MultiKeyMap
1. cachedHashCode not invalidated by ConcurrentMap methods (putIfAbsent, computeIfAbsent, computeIfPresent, compute, merge, remove(K,V), replace(K,V), replace(K,V,V)) - added cachedHashCode = null to each 2. compareCollections skips trailing elements after Set sections - the unconditional i++ at end of while loop overcounts after Set branch which already advances i; fixed with continue 3. Small Set comparison (<=6 elements) does not track consumed matches - under valueBasedEquality, two elements in set1 could match the same element in set2; fixed with boolean[] consumed tracking in all three comparison methods 4. Stripe contention diagnostics track wrong stripe - putInternal and removeInternal used hash & STRIPE_MASK but getStripeLock uses (hash & tableMask) & STRIPE_MASK; extracted getStripeIndex() helper 5. ThreadLocal lookup arrays leak references - getMultiKey and containsMultiKey never null out array entries after use; added try/finally cleanup in all 8 methods Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 49404dd commit 6dd6d79

7 files changed

Lines changed: 885 additions & 32 deletions

changelog.md

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,24 @@
2424
* Increased threshold from 2ms to 50ms for CI environments with thread scheduling delays
2525
* **MAINTENANCE**: Fixed flaky `MultiKeyMapLockStripingTest.testPerformanceWithStriping` test
2626
* Increased slowdown tolerance from 5x to 10x for CI environments with shared resources
27+
* **BUG FIX**: `MultiKeyMap` - `cachedHashCode` not invalidated by ConcurrentMap methods
28+
* `putIfAbsent`, `computeIfAbsent`, `computeIfPresent`, `compute`, `merge`, `remove(K,V)`, `replace(K,V)`, `replace(K,V,V)` all bypassed `cachedHashCode = null` invalidation
29+
* After any of these methods mutated the map, `hashCode()` returned a stale cached value
30+
* **BUG FIX**: `MultiKeyMap` - `compareCollections` skips trailing elements after Set sections
31+
* Unconditional `i++` at end of while loop overcounted after Set branch which already advances `i`
32+
* Elements following a Set in an expanded key were never compared, causing false key matches
33+
* **BUG FIX**: `MultiKeyMap` - Small Set comparison (<=6 elements) doesn't track consumed matches
34+
* Under `valueBasedEquality`, two distinct elements in set1 could match the same element in set2
35+
* For example, `Integer(1)` and `Long(1L)` both matching `Integer(1)`, producing false equality
36+
* Fixed with `boolean[]` consumed tracking in all three comparison methods
37+
* **BUG FIX**: `MultiKeyMap` - Stripe contention diagnostics track wrong stripe
38+
* `putInternal` and `removeInternal` used `hash & STRIPE_MASK` but `getStripeLock` uses `(hash & tableMask) & STRIPE_MASK`
39+
* When table size < stripe count, per-stripe metrics were attributed to incorrect stripes
40+
* Extracted shared `getStripeIndex()` helper for consistent stripe computation
41+
* **BUG FIX**: `MultiKeyMap` - ThreadLocal lookup arrays leak references
42+
* `getMultiKey()` and `containsMultiKey()` methods never nulled out array entries after use
43+
* In thread-pool environments, this pinned references to user objects for the lifetime of the thread
44+
* Added `try/finally` cleanup in all 8 methods
2745

2846
#### 4.90.0 2026-02-02
2947
* **BUG FIX**: `DeepEquals` - URL comparison now uses string representation instead of `URL.equals()`

src/main/java/com/cedarsoftware/util/MultiKeyMap.java

Lines changed: 121 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -969,13 +969,16 @@ private static int hashDouble(double d) {
969969
return (int) (bits ^ (bits >>> 32));
970970
}
971971

972-
private ReentrantLock getStripeLock(int hash) {
973-
// GPT5 optimization: Use bucket index for stripe selection to reduce false contention
972+
private int getStripeIndex(int hash) {
973+
// Use bucket index for stripe selection to reduce false contention
974974
// between independent buckets that happen to have same low-order hash bits
975975
final AtomicReferenceArray<MultiKey<V>[]> table = buckets; // Volatile read of buckets
976976
final int mask = table.length() - 1; // Array length is immutable
977-
int bucketIndex = hash & mask;
978-
return stripeLocks[bucketIndex & STRIPE_MASK];
977+
return (hash & mask) & STRIPE_MASK;
978+
}
979+
980+
private ReentrantLock getStripeLock(int hash) {
981+
return stripeLocks[getStripeIndex(hash)];
979982
}
980983

981984
private void lockAllStripes() {
@@ -1024,7 +1027,12 @@ public V getMultiKey(Object k1, Object k2) {
10241027
Object[] key = LOOKUP_KEY_2.get();
10251028
key[0] = k1;
10261029
key[1] = k2;
1027-
return get(key);
1030+
try {
1031+
return get(key);
1032+
} finally {
1033+
key[0] = null;
1034+
key[1] = null;
1035+
}
10281036
}
10291037

10301038
/**
@@ -1040,7 +1048,13 @@ public V getMultiKey(Object k1, Object k2, Object k3) {
10401048
key[0] = k1;
10411049
key[1] = k2;
10421050
key[2] = k3;
1043-
return get(key);
1051+
try {
1052+
return get(key);
1053+
} finally {
1054+
key[0] = null;
1055+
key[1] = null;
1056+
key[2] = null;
1057+
}
10441058
}
10451059

10461060
/**
@@ -1058,7 +1072,14 @@ public V getMultiKey(Object k1, Object k2, Object k3, Object k4) {
10581072
key[1] = k2;
10591073
key[2] = k3;
10601074
key[3] = k4;
1061-
return get(key);
1075+
try {
1076+
return get(key);
1077+
} finally {
1078+
key[0] = null;
1079+
key[1] = null;
1080+
key[2] = null;
1081+
key[3] = null;
1082+
}
10621083
}
10631084

10641085
/**
@@ -1078,7 +1099,15 @@ public V getMultiKey(Object k1, Object k2, Object k3, Object k4, Object k5) {
10781099
key[2] = k3;
10791100
key[3] = k4;
10801101
key[4] = k5;
1081-
return get(key);
1102+
try {
1103+
return get(key);
1104+
} finally {
1105+
key[0] = null;
1106+
key[1] = null;
1107+
key[2] = null;
1108+
key[3] = null;
1109+
key[4] = null;
1110+
}
10821111
}
10831112

10841113
/**
@@ -2319,12 +2348,15 @@ private boolean compareObjectArrays(Object[] array1, Object[] array2, int size)
23192348
// Empty sets - equal, nothing to compare
23202349
// No action needed - continue to next element
23212350
} else if (setSize <= 6) {
2322-
// Optimization: For small Sets (≤6 elements), nested loop is faster than allocation
2351+
// Optimization: For small Sets (≤6 elements), nested loop is faster than HashMap
23232352
// O(n²) comparison (max 36 operations) but avoids HashMap overhead
2353+
// Track consumed matches to prevent double-matching under valueBasedEquality
2354+
boolean[] consumed = new boolean[setSize];
23242355
for (int s1 = startIdx; s1 < startIdx + setSize; s1++) {
23252356
boolean found = false;
23262357
for (int s2 = startIdx; s2 < startIdx + setSize; s2++) {
2327-
if (elementEquals(array1[s1], array2[s2], valueBasedEquality, caseSensitive)) {
2358+
if (!consumed[s2 - startIdx] && elementEquals(array1[s1], array2[s2], valueBasedEquality, caseSensitive)) {
2359+
consumed[s2 - startIdx] = true;
23282360
found = true;
23292361
break;
23302362
}
@@ -2459,10 +2491,13 @@ private static boolean compareObjectArrayToCollection(Object[] array, Collection
24592491
} else if (setSize <= 6) {
24602492
// Optimization: For small Sets (≤6 elements), nested loop is faster than allocation
24612493
// O(n²) comparison (max 36 operations) - we only allocate the iterator list
2494+
// Track consumed matches to prevent double-matching under valueBasedEquality
2495+
boolean[] consumed = new boolean[setSize];
24622496
for (int s1 = arrayStartIdx; s1 < arrayStartIdx + arraySetSize; s1++) {
24632497
boolean found = false;
2464-
for (Object iterElem : iterElements) {
2465-
if (elementEquals(array[s1], iterElem, valueBasedEquality, caseSensitive)) {
2498+
for (int s2 = 0; s2 < iterElements.size(); s2++) {
2499+
if (!consumed[s2] && elementEquals(array[s1], iterElements.get(s2), valueBasedEquality, caseSensitive)) {
2500+
consumed[s2] = true;
24662501
found = true;
24672502
break;
24682503
}
@@ -2616,10 +2651,13 @@ private static boolean compareCollections(Collection<?> coll1, Collection<?> col
26162651
} else if (setSize <= 6) {
26172652
// Optimization: For small Sets (≤6 elements), nested loop is faster than additional allocation
26182653
// O(n²) comparison (max 36 operations) - we already have the two lists
2619-
for (Object setElem1 : set1Elements) {
2654+
// Track consumed matches to prevent double-matching under valueBasedEquality
2655+
boolean[] consumed = new boolean[setSize];
2656+
for (int s1 = 0; s1 < set1Elements.size(); s1++) {
26202657
boolean found = false;
2621-
for (Object setElem2 : set2Elements) {
2622-
if (elementEquals(setElem1, setElem2, valueBasedEquality, caseSensitive)) {
2658+
for (int s2 = 0; s2 < set2Elements.size(); s2++) {
2659+
if (!consumed[s2] && elementEquals(set1Elements.get(s1), set2Elements.get(s2), valueBasedEquality, caseSensitive)) {
2660+
consumed[s2] = true;
26232661
found = true;
26242662
break;
26252663
}
@@ -2688,6 +2726,7 @@ private static boolean compareCollections(Collection<?> coll1, Collection<?> col
26882726
}
26892727

26902728
// Note: SET_CLOSE was already counted when extracted (line 2529), no need to count again
2729+
continue; // Skip the unconditional i++ below; the Set branch already advanced i correctly
26912730
} else if (elem1 == SET_OPEN || elem2 == SET_OPEN) {
26922731
// One is SET_OPEN but not the other - mismatch
26932732
return false;
@@ -3060,8 +3099,8 @@ private V putInternal(MultiKey<V> newKey) {
30603099
cachedHashCode = null;
30613100

30623101
int hash = newKey.hash;
3063-
ReentrantLock lock = getStripeLock(hash);
3064-
int stripe = hash & STRIPE_MASK;
3102+
int stripe = getStripeIndex(hash);
3103+
ReentrantLock lock = stripeLocks[stripe];
30653104
V old;
30663105
boolean resize;
30673106

@@ -3073,7 +3112,7 @@ private V putInternal(MultiKey<V> newKey) {
30733112
contentionCount.incrementAndGet();
30743113
stripeLockContention[stripe].incrementAndGet();
30753114
}
3076-
3115+
30773116
try {
30783117
totalLockAcquisitions.incrementAndGet();
30793118
stripeLockAcquisitions[stripe].incrementAndGet();
@@ -3169,7 +3208,12 @@ public boolean containsMultiKey(Object k1, Object k2) {
31693208
Object[] key = LOOKUP_KEY_2.get();
31703209
key[0] = k1;
31713210
key[1] = k2;
3172-
return containsKey(key);
3211+
try {
3212+
return containsKey(key);
3213+
} finally {
3214+
key[0] = null;
3215+
key[1] = null;
3216+
}
31733217
}
31743218

31753219
/**
@@ -3185,7 +3229,13 @@ public boolean containsMultiKey(Object k1, Object k2, Object k3) {
31853229
key[0] = k1;
31863230
key[1] = k2;
31873231
key[2] = k3;
3188-
return containsKey(key);
3232+
try {
3233+
return containsKey(key);
3234+
} finally {
3235+
key[0] = null;
3236+
key[1] = null;
3237+
key[2] = null;
3238+
}
31893239
}
31903240

31913241
/**
@@ -3203,7 +3253,14 @@ public boolean containsMultiKey(Object k1, Object k2, Object k3, Object k4) {
32033253
key[1] = k2;
32043254
key[2] = k3;
32053255
key[3] = k4;
3206-
return containsKey(key);
3256+
try {
3257+
return containsKey(key);
3258+
} finally {
3259+
key[0] = null;
3260+
key[1] = null;
3261+
key[2] = null;
3262+
key[3] = null;
3263+
}
32073264
}
32083265

32093266
/**
@@ -3223,7 +3280,15 @@ public boolean containsMultiKey(Object k1, Object k2, Object k3, Object k4, Obje
32233280
key[2] = k3;
32243281
key[3] = k4;
32253282
key[4] = k5;
3226-
return containsKey(key);
3283+
try {
3284+
return containsKey(key);
3285+
} finally {
3286+
key[0] = null;
3287+
key[1] = null;
3288+
key[2] = null;
3289+
key[3] = null;
3290+
key[4] = null;
3291+
}
32273292
}
32283293

32293294
/**
@@ -3276,8 +3341,8 @@ private V removeInternal(final MultiKey<V> removeKey) {
32763341
cachedHashCode = null;
32773342

32783343
int hash = removeKey.hash;
3279-
ReentrantLock lock = getStripeLock(hash);
3280-
int stripe = hash & STRIPE_MASK;
3344+
int stripe = getStripeIndex(hash);
3345+
ReentrantLock lock = stripeLocks[stripe];
32813346
V old;
32823347

32833348
// Use tryLock() to accurately detect contention
@@ -3288,7 +3353,7 @@ private V removeInternal(final MultiKey<V> removeKey) {
32883353
contentionCount.incrementAndGet();
32893354
stripeLockContention[stripe].incrementAndGet();
32903355
}
3291-
3356+
32923357
try {
32933358
totalLockAcquisitions.incrementAndGet();
32943359
stripeLockAcquisitions[stripe].incrementAndGet();
@@ -3596,16 +3661,19 @@ public void putAll(Map<?, ? extends V> m) {
35963661
* if there was no mapping for the key
35973662
*/
35983663
public V putIfAbsent(Object key, V value) {
3664+
// Invalidate hashCode cache on potential mutation
3665+
cachedHashCode = null;
3666+
35993667
V existing = get(key);
36003668
if (existing != null) return existing;
3601-
3669+
36023670
// Normalize the key once, outside the lock
36033671
MultiKey<V> norm = flattenKey(key);
36043672
Object normalizedKey = norm.keys;
36053673
int hash = norm.hash;
36063674
ReentrantLock lock = getStripeLock(hash);
36073675
boolean resize = false;
3608-
3676+
36093677
lock.lock();
36103678
try {
36113679
// Check again inside the lock
@@ -3640,6 +3708,9 @@ public V putIfAbsent(Object key, V value) {
36403708
public V computeIfAbsent(Object key, Function<? super Object, ? extends V> mappingFunction) {
36413709
Objects.requireNonNull(mappingFunction);
36423710

3711+
// Invalidate hashCode cache on potential mutation
3712+
cachedHashCode = null;
3713+
36433714
// Normalize key once upfront - avoid double normalization that get() + flattenKey() would cause
36443715
MultiKey<V> norm = flattenKey(key);
36453716
Object normalizedKey = norm.keys;
@@ -3692,6 +3763,9 @@ public V computeIfAbsent(Object key, Function<? super Object, ? extends V> mappi
36923763
public V computeIfPresent(Object key, BiFunction<? super Object, ? super V, ? extends V> remappingFunction) {
36933764
Objects.requireNonNull(remappingFunction);
36943765

3766+
// Invalidate hashCode cache on potential mutation
3767+
cachedHashCode = null;
3768+
36953769
// Normalize key once upfront - avoid double normalization that get() + flattenKey() would cause
36963770
MultiKey<V> norm = flattenKey(key);
36973771
Object normalizedKey = norm.keys;
@@ -3747,6 +3821,9 @@ public V computeIfPresent(Object key, BiFunction<? super Object, ? super V, ? ex
37473821
public V compute(Object key, BiFunction<? super Object, ? super V, ? extends V> remappingFunction) {
37483822
Objects.requireNonNull(remappingFunction);
37493823

3824+
// Invalidate hashCode cache on potential mutation
3825+
cachedHashCode = null;
3826+
37503827
MultiKey<V> norm = flattenKey(key);
37513828
Object normalizedKey = norm.keys;
37523829
int hash = norm.hash;
@@ -3798,13 +3875,16 @@ public V compute(Object key, BiFunction<? super Object, ? super V, ? extends V>
37983875
public V merge(Object key, V value, BiFunction<? super V, ? super V, ? extends V> remappingFunction) {
37993876
Objects.requireNonNull(value);
38003877
Objects.requireNonNull(remappingFunction);
3801-
3878+
3879+
// Invalidate hashCode cache on potential mutation
3880+
cachedHashCode = null;
3881+
38023882
MultiKey<V> norm = flattenKey(key);
38033883
Object normalizedKey = norm.keys;
38043884
int hash = norm.hash;
38053885
ReentrantLock lock = getStripeLock(hash);
38063886
boolean resize = false;
3807-
3887+
38083888
V result;
38093889
lock.lock();
38103890
try {
@@ -3848,11 +3928,14 @@ public V merge(Object key, V value, BiFunction<? super V, ? super V, ? extends V
38483928
* @return {@code true} if the value was removed
38493929
*/
38503930
public boolean remove(Object key, Object value) {
3931+
// Invalidate hashCode cache on potential mutation
3932+
cachedHashCode = null;
3933+
38513934
MultiKey<V> norm = flattenKey(key);
38523935
Object normalizedKey = norm.keys;
38533936
int hash = norm.hash;
38543937
ReentrantLock lock = getStripeLock(hash);
3855-
3938+
38563939
lock.lock();
38573940
try {
38583941
MultiKey<V> lookupKey = new MultiKey<>(normalizedKey, hash, null);
@@ -3885,12 +3968,15 @@ public boolean remove(Object key, Object value) {
38853968
* if there was no mapping for the key
38863969
*/
38873970
public V replace(Object key, V value) {
3971+
// Invalidate hashCode cache on potential mutation
3972+
cachedHashCode = null;
3973+
38883974
MultiKey<V> norm = flattenKey(key);
38893975
Object normalizedKey = norm.keys;
38903976
int hash = norm.hash;
38913977
ReentrantLock lock = getStripeLock(hash);
38923978
boolean resize = false;
3893-
3979+
38943980
V result;
38953981
lock.lock();
38963982
try {
@@ -3930,12 +4016,15 @@ public V replace(Object key, V value) {
39304016
* @return {@code true} if the value was replaced
39314017
*/
39324018
public boolean replace(Object key, V oldValue, V newValue) {
4019+
// Invalidate hashCode cache on potential mutation
4020+
cachedHashCode = null;
4021+
39334022
MultiKey<V> norm = flattenKey(key);
39344023
Object normalizedKey = norm.keys;
39354024
int hash = norm.hash;
39364025
ReentrantLock lock = getStripeLock(hash);
39374026
boolean resize = false;
3938-
4027+
39394028
boolean result = false;
39404029
lock.lock();
39414030
try {

0 commit comments

Comments
 (0)