From 3fa56db88b62483183aa941d01a2a24d4fc0fcd0 Mon Sep 17 00:00:00 2001 From: Patrick Strawderman Date: Thu, 13 Nov 2025 11:51:39 -0800 Subject: [PATCH] Fix Spliterator characteristics in ConcurrentReferenceHashMap The Spliterators returned by values, entrySet, and keySet incorrectly reported the SIZED characteristic, instead of CONCURRENT. This could lead to bugs when the map is concurrently modified during a stream operation. For keySet and values, the incorrect characteristics are inherited from AbstractMap, so to rectify that the respective methods are overridden, and custom collections are provided that report the correct Spliterator characteristics. Closes gh-35817 Signed-off-by: Patrick Strawderman (cherry picked from commit ed7590683467efcea27507bd5cfab6acc006239c) --- .../util/ConcurrentReferenceHashMap.java | 160 ++++++++++++++- .../util/ConcurrentReferenceHashMapTests.java | 194 ++++++++++++++++-- 2 files changed, 339 insertions(+), 15 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/util/ConcurrentReferenceHashMap.java b/spring-core/src/main/java/org/springframework/util/ConcurrentReferenceHashMap.java index 7ebcb78aa80..b4daff966d7 100644 --- a/spring-core/src/main/java/org/springframework/util/ConcurrentReferenceHashMap.java +++ b/spring-core/src/main/java/org/springframework/util/ConcurrentReferenceHashMap.java @@ -20,8 +20,10 @@ import java.lang.ref.ReferenceQueue; import java.lang.ref.SoftReference; import java.lang.ref.WeakReference; import java.lang.reflect.Array; +import java.util.AbstractCollection; import java.util.AbstractMap; import java.util.AbstractSet; +import java.util.Collection; import java.util.Collections; import java.util.EnumSet; import java.util.HashSet; @@ -29,6 +31,8 @@ import java.util.Iterator; import java.util.Map; import java.util.NoSuchElementException; import java.util.Set; +import java.util.Spliterator; +import java.util.Spliterators; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.atomic.AtomicInteger; @@ -104,6 +108,18 @@ public class ConcurrentReferenceHashMap extends AbstractMap implemen @Nullable private volatile Set> entrySet; + /** + * Late binding key set. + */ + @Nullable + private Set keySet; + + /** + * Late binding values collection. + */ + @Nullable + private Collection values; + /** * Create a new {@code ConcurrentReferenceHashMap} instance. @@ -528,6 +544,26 @@ public class ConcurrentReferenceHashMap extends AbstractMap implemen return entrySet; } + @Override + public Set keySet() { + Set keySet = this.keySet; + if (keySet == null) { + keySet = new KeySet(); + this.keySet = keySet; + } + return keySet; + } + + @Override + public Collection values() { + Collection values = this.values; + if (values == null) { + values = new Values(); + this.values = values; + } + return values; + } + @Nullable private T doTask(@Nullable Object key, Task task) { int hash = getHash(key); @@ -969,7 +1005,7 @@ public class ConcurrentReferenceHashMap extends AbstractMap implemen /** * Internal entry-set implementation. */ - private class EntrySet extends AbstractSet> { + private final class EntrySet extends AbstractSet> { @Override public Iterator> iterator() { @@ -1005,13 +1041,133 @@ public class ConcurrentReferenceHashMap extends AbstractMap implemen public void clear() { ConcurrentReferenceHashMap.this.clear(); } + + @Override + public Spliterator> spliterator() { + return Spliterators.spliterator(this, Spliterator.DISTINCT | Spliterator.CONCURRENT); + } + } + + /** + * Internal key-set implementation. + */ + private final class KeySet extends AbstractSet { + @Override + public Iterator iterator() { + return new KeyIterator(); + } + + @Override + public int size() { + return ConcurrentReferenceHashMap.this.size(); + } + + @Override + public boolean isEmpty() { + return ConcurrentReferenceHashMap.this.isEmpty(); + } + + @Override + public void clear() { + ConcurrentReferenceHashMap.this.clear(); + } + + @Override + public boolean contains(Object k) { + return ConcurrentReferenceHashMap.this.containsKey(k); + } + + @Override + public Spliterator spliterator() { + return Spliterators.spliterator(this, Spliterator.DISTINCT | Spliterator.CONCURRENT); + } + } + + /** + * Internal key iterator implementation. + */ + private final class KeyIterator implements Iterator { + + private final Iterator> iterator = entrySet().iterator(); + + @Override + public boolean hasNext() { + return this.iterator.hasNext(); + } + + @Override + public void remove() { + this.iterator.remove(); + } + + @Override + public K next() { + return this.iterator.next().getKey(); + } + } + + /** + * Internal values collection implementation. + */ + private final class Values extends AbstractCollection { + @Override + public Iterator iterator() { + return new ValueIterator(); + } + + @Override + public int size() { + return ConcurrentReferenceHashMap.this.size(); + } + + @Override + public boolean isEmpty() { + return ConcurrentReferenceHashMap.this.isEmpty(); + } + + @Override + public void clear() { + ConcurrentReferenceHashMap.this.clear(); + } + + @Override + public boolean contains(Object v) { + return ConcurrentReferenceHashMap.this.containsValue(v); + } + + @Override + public Spliterator spliterator() { + return Spliterators.spliterator(this, Spliterator.CONCURRENT); + } } + /** + * Internal value iterator implementation. + */ + private final class ValueIterator implements Iterator { + + private final Iterator> iterator = entrySet().iterator(); + + @Override + public boolean hasNext() { + return this.iterator.hasNext(); + } + + @Override + public void remove() { + this.iterator.remove(); + } + + @Override + public V next() { + return this.iterator.next().getValue(); + } + } /** * Internal entry iterator implementation. */ - private class EntryIterator implements Iterator> { + private final class EntryIterator implements Iterator> { private int segmentIndex; diff --git a/spring-core/src/test/java/org/springframework/util/ConcurrentReferenceHashMapTests.java b/spring-core/src/test/java/org/springframework/util/ConcurrentReferenceHashMapTests.java index 5a001e74b7b..8e67f3bd44f 100644 --- a/spring-core/src/test/java/org/springframework/util/ConcurrentReferenceHashMapTests.java +++ b/spring-core/src/test/java/org/springframework/util/ConcurrentReferenceHashMapTests.java @@ -16,8 +16,8 @@ package org.springframework.util; -import java.util.ArrayList; -import java.util.Comparator; +import java.util.AbstractMap; +import java.util.Collection; import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; @@ -25,6 +25,8 @@ import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.Spliterator; +import java.util.stream.Collectors; import org.junit.jupiter.api.Test; @@ -32,12 +34,12 @@ import org.springframework.lang.Nullable; import org.springframework.util.ConcurrentReferenceHashMap.Entry; import org.springframework.util.ConcurrentReferenceHashMap.Reference; import org.springframework.util.ConcurrentReferenceHashMap.Restructure; -import org.springframework.util.comparator.Comparators; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import static org.assertj.core.api.Assertions.assertThatIllegalStateException; import static org.assertj.core.api.Assertions.assertThatNoException; +import static org.assertj.core.api.Assertions.assertThatThrownBy; /** * Tests for {@link ConcurrentReferenceHashMap}. @@ -47,8 +49,6 @@ import static org.assertj.core.api.Assertions.assertThatNoException; */ class ConcurrentReferenceHashMapTests { - private static final Comparator NULL_SAFE_STRING_SORT = Comparators.nullsLow(); - private TestWeakConcurrentCache map = new TestWeakConcurrentCache<>(); @@ -450,19 +450,176 @@ class ConcurrentReferenceHashMapTests { assertThat(this.map.keySet()).isEqualTo(expected); } + @Test + void keySetContains() { + this.map.put(123, "123"); + this.map.put(456, null); + this.map.put(null, "789"); + assertThat(this.map.keySet()).containsExactlyInAnyOrder(123, 456, null); + } + + @Test + void keySetRemove() { + this.map.put(123, "123"); + this.map.put(456, null); + this.map.put(null, "789"); + assertThat(this.map.keySet().remove(123)).isTrue(); + assertThat(this.map).doesNotContainKey(123); + assertThat(this.map.keySet().remove(123)).isFalse(); + } + + @Test + void keySetIterator() { + this.map.put(123, "123"); + this.map.put(456, null); + this.map.put(null, "789"); + Iterator it = this.map.keySet().iterator(); + assertThat(it).toIterable().containsExactlyInAnyOrder(123, 456, null); + assertThat(it).isExhausted(); + } + + @Test + void keySetIteratorRemove() { + this.map.put(123, "123"); + this.map.put(456, null); + this.map.put(null, "789"); + Iterator keySetIterator = this.map.keySet().iterator(); + while (keySetIterator.hasNext()) { + Integer key = keySetIterator.next(); + if (key != null && key.equals(456)) { + keySetIterator.remove(); + } + } + assertThat(this.map).containsOnlyKeys(123, null); + } + + @Test + void keySetClear() { + this.map.put(123, "123"); + this.map.put(456, null); + this.map.put(null, "789"); + this.map.keySet().clear(); + assertThat(this.map).isEmpty(); + assertThat(this.map.keySet()).isEmpty(); + } + + @Test + void keySetAdd() { + assertThatThrownBy(() -> + this.map.keySet().add(12345) + ).isInstanceOf(UnsupportedOperationException.class); + } + + @Test + void keySetStream() { + this.map.put(123, "123"); + this.map.put(456, null); + this.map.put(null, "789"); + Set keys = this.map.keySet().stream().collect(Collectors.toSet()); + assertThat(keys).containsExactlyInAnyOrder(123, 456, null); + } + + @Test + void keySetSpliteratorCharacteristics() { + this.map.put(123, "123"); + this.map.put(456, null); + this.map.put(null, "789"); + Spliterator spliterator = this.map.keySet().spliterator(); + assertThat(spliterator).hasOnlyCharacteristics(Spliterator.CONCURRENT, Spliterator.DISTINCT); + assertThat(spliterator.estimateSize()).isEqualTo(3L); + assertThat(spliterator.getExactSizeIfKnown()).isEqualTo(-1L); + } + @Test void valuesCollection() { this.map.put(123, "123"); this.map.put(456, null); this.map.put(null, "789"); - List actual = new ArrayList<>(this.map.values()); - List expected = new ArrayList<>(); - expected.add("123"); - expected.add(null); - expected.add("789"); - actual.sort(NULL_SAFE_STRING_SORT); - expected.sort(NULL_SAFE_STRING_SORT); - assertThat(actual).isEqualTo(expected); + assertThat(this.map.values()).containsExactlyInAnyOrder("123", null, "789"); + } + + @Test + void valuesCollectionAdd() { + assertThatThrownBy(() -> + this.map.values().add("12345") + ).isInstanceOf(UnsupportedOperationException.class); + } + + @Test + void valuesCollectionClear() { + Collection values = this.map.values(); + this.map.put(123, "123"); + this.map.put(456, null); + this.map.put(null, "789"); + assertThat(values).isNotEmpty(); + values.clear(); + assertThat(values).isEmpty(); + assertThat(this.map).isEmpty(); + } + + @Test + void valuesCollectionRemoval() { + Collection values = this.map.values(); + assertThat(values).isEmpty(); + this.map.put(123, "123"); + this.map.put(456, null); + this.map.put(null, "789"); + assertThat(values).containsExactlyInAnyOrder("123", null, "789"); + values.remove(null); + assertThat(values).containsExactlyInAnyOrder("123", "789"); + assertThat(map).containsOnly(new AbstractMap.SimpleEntry<>(123, "123"), new AbstractMap.SimpleEntry<>(null, "789")); + values.remove("123"); + values.remove("789"); + assertThat(values).isEmpty(); + assertThat(map).isEmpty(); + } + + @Test + void valuesCollectionIterator() { + Iterator iterator = this.map.values().iterator(); + assertThat(iterator).isExhausted(); + this.map.put(123, "123"); + this.map.put(456, null); + this.map.put(null, "789"); + iterator = this.map.values().iterator(); + assertThat(iterator).toIterable().containsExactlyInAnyOrder("123", null, "789"); + } + + @Test + void valuesCollectionIteratorRemoval() { + this.map.put(123, "123"); + this.map.put(456, null); + this.map.put(null, "789"); + Iterator iterator = this.map.values().iterator(); + while (iterator.hasNext()) { + String value = iterator.next(); + if (value != null && value.equals("789")) { + iterator.remove(); + } + } + assertThat(iterator).isExhausted(); + assertThat(this.map.values()).containsExactlyInAnyOrder("123", null); + assertThat(this.map).containsOnlyKeys(123, 456); + } + + @Test + void valuesCollectionStream() { + this.map.put(123, "123"); + this.map.put(456, null); + this.map.put(null, "789"); + List values = this.map.values().stream().collect(Collectors.toList()); + assertThat(values).containsExactlyInAnyOrder("123", null, "789"); + } + + @Test + void valuesCollectionSpliteratorCharacteristics() { + this.map.put(123, "123"); + this.map.put(456, null); + this.map.put(null, "789"); + Spliterator spliterator = this.map.values().spliterator(); + assertThat(spliterator).hasOnlyCharacteristics(Spliterator.CONCURRENT); + assertThat(spliterator.estimateSize()).isEqualTo(3L); + assertThat(spliterator.getExactSizeIfKnown()).isEqualTo(-1L); } @Test @@ -541,6 +698,17 @@ class ConcurrentReferenceHashMapTests { copy.forEach(entry -> assertThat(entrySet).doesNotContain(entry)); } + @Test + void entrySetSpliteratorCharacteristics() { + this.map.put(1, "1"); + this.map.put(2, "2"); + this.map.put(3, "3"); + Spliterator> spliterator = this.map.entrySet().spliterator(); + assertThat(spliterator).hasOnlyCharacteristics(Spliterator.CONCURRENT, Spliterator.DISTINCT); + assertThat(spliterator.estimateSize()).isEqualTo(3L); + assertThat(spliterator.getExactSizeIfKnown()).isEqualTo(-1L); + } + @Test void supportNullReference() { // GC could happen during restructure so we must be able to create a reference for a null entry