mirror of
https://github.com/spring-projects/spring-framework.git
synced 2026-09-20 04:09:04 +00:00
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 <pstrawderman@netflix.com>
(cherry picked from commit ed75906834)
This commit is contained in:
Patrick Strawderman
committed by
Sam Brannen
Sam Brannen
parent
ed444eb0c1
commit
3fa56db88b
+158
-2
@@ -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<K, V> extends AbstractMap<K, V> implemen
|
||||
@Nullable
|
||||
private volatile Set<Map.Entry<K, V>> entrySet;
|
||||
|
||||
/**
|
||||
* Late binding key set.
|
||||
*/
|
||||
@Nullable
|
||||
private Set<K> keySet;
|
||||
|
||||
/**
|
||||
* Late binding values collection.
|
||||
*/
|
||||
@Nullable
|
||||
private Collection<V> values;
|
||||
|
||||
|
||||
/**
|
||||
* Create a new {@code ConcurrentReferenceHashMap} instance.
|
||||
@@ -528,6 +544,26 @@ public class ConcurrentReferenceHashMap<K, V> extends AbstractMap<K, V> implemen
|
||||
return entrySet;
|
||||
}
|
||||
|
||||
@Override
|
||||
public Set<K> keySet() {
|
||||
Set<K> keySet = this.keySet;
|
||||
if (keySet == null) {
|
||||
keySet = new KeySet();
|
||||
this.keySet = keySet;
|
||||
}
|
||||
return keySet;
|
||||
}
|
||||
|
||||
@Override
|
||||
public Collection<V> values() {
|
||||
Collection<V> values = this.values;
|
||||
if (values == null) {
|
||||
values = new Values();
|
||||
this.values = values;
|
||||
}
|
||||
return values;
|
||||
}
|
||||
|
||||
@Nullable
|
||||
private <T> T doTask(@Nullable Object key, Task<T> task) {
|
||||
int hash = getHash(key);
|
||||
@@ -969,7 +1005,7 @@ public class ConcurrentReferenceHashMap<K, V> extends AbstractMap<K, V> implemen
|
||||
/**
|
||||
* Internal entry-set implementation.
|
||||
*/
|
||||
private class EntrySet extends AbstractSet<Map.Entry<K, V>> {
|
||||
private final class EntrySet extends AbstractSet<Map.Entry<K, V>> {
|
||||
|
||||
@Override
|
||||
public Iterator<Map.Entry<K, V>> iterator() {
|
||||
@@ -1005,13 +1041,133 @@ public class ConcurrentReferenceHashMap<K, V> extends AbstractMap<K, V> implemen
|
||||
public void clear() {
|
||||
ConcurrentReferenceHashMap.this.clear();
|
||||
}
|
||||
|
||||
@Override
|
||||
public Spliterator<Map.Entry<K, V>> spliterator() {
|
||||
return Spliterators.spliterator(this, Spliterator.DISTINCT | Spliterator.CONCURRENT);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Internal key-set implementation.
|
||||
*/
|
||||
private final class KeySet extends AbstractSet<K> {
|
||||
@Override
|
||||
public Iterator<K> 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<K> spliterator() {
|
||||
return Spliterators.spliterator(this, Spliterator.DISTINCT | Spliterator.CONCURRENT);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Internal key iterator implementation.
|
||||
*/
|
||||
private final class KeyIterator implements Iterator<K> {
|
||||
|
||||
private final Iterator<Map.Entry<K, V>> 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<V> {
|
||||
@Override
|
||||
public Iterator<V> 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<V> spliterator() {
|
||||
return Spliterators.spliterator(this, Spliterator.CONCURRENT);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Internal value iterator implementation.
|
||||
*/
|
||||
private final class ValueIterator implements Iterator<V> {
|
||||
|
||||
private final Iterator<Map.Entry<K, V>> 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<Map.Entry<K, V>> {
|
||||
private final class EntryIterator implements Iterator<Map.Entry<K, V>> {
|
||||
|
||||
private int segmentIndex;
|
||||
|
||||
|
||||
+181
-13
@@ -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<? super String> NULL_SAFE_STRING_SORT = Comparators.nullsLow();
|
||||
|
||||
private TestWeakConcurrentCache<Integer, String> 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<Integer> 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<Integer> 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<Integer> 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<Integer> 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<String> actual = new ArrayList<>(this.map.values());
|
||||
List<String> 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<String> 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<String> 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<String> 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<String> 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<String> 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<String> 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<Map.Entry<Integer, String>> 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
|
||||
|
||||
Reference in New Issue
Block a user