root
/
spring-framework
mirror of https://github.com/spring-projects/spring-framework.git
1
0
Fork 0
Code Issues Actions 10 Packages Projects Releases Wiki Activity

ConcurrentReferenceHashMap properly handles getOrDefault for null values 

Issue: SPR-16584
This commit is contained in:
Juergen Hoeller 2018-03-14 17:17:04 +01:00
parent cc12afdea2
commit 356ef45e99
2 changed files with 96 additions and 86 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

59
spring-core/src/main/java/org/springframework/util/ConcurrentReferenceHashMap.java
View File

@ -1,5 +1,5 @@
/*
* Copyright 2002-2017 the original author or authors.
* Copyright 2002-2018 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@ -54,6 +54,7 @@ import org.springframework.lang.Nullable;
* {@linkplain SoftReference soft entry references}.
*
* @author Phillip Webb
* @author Juergen Hoeller
* @since 3.2
* @param <K> the key type
* @param <V> the value type
@ -226,19 +227,30 @@ public class ConcurrentReferenceHashMap<K, V> extends AbstractMap<K, V> implemen
@Override
@Nullable
public V get(Object key) {
Reference<K, V> reference = getReference(key, Restructure.WHEN_NECESSARY);
Entry<K, V> entry = (reference != null ? reference.get() : null);
public V get(@Nullable Object key) {
Entry<K, V> entry = getEntryIfAvailable(key);
return (entry != null ? entry.getValue() : null);
}
@Override
public boolean containsKey(Object key) {
Reference<K, V> reference = getReference(key, Restructure.WHEN_NECESSARY);
Entry<K, V> entry = (reference != null ? reference.get() : null);
@Nullable
public V getOrDefault(@Nullable Object key, @Nullable V defaultValue) {
Entry<K, V> entry = getEntryIfAvailable(key);
return (entry != null ? entry.getValue() : defaultValue);
}
@Override
public boolean containsKey(@Nullable Object key) {
Entry<K, V> entry = getEntryIfAvailable(key);
return (entry != null && ObjectUtils.nullSafeEquals(entry.getKey(), key));
}
@Nullable
private Entry<K, V> getEntryIfAvailable(@Nullable Object key) {
Reference<K, V> reference = getReference(key, Restructure.WHEN_NECESSARY);
return (reference != null ? reference.get() : null);
}
/**
* Return a {@link Reference} to the {@link Entry} for the specified {@code key},
* or {@code null} if not found.
@ -254,28 +266,28 @@ public class ConcurrentReferenceHashMap<K, V> extends AbstractMap<K, V> implemen
@Override
@Nullable
public V put(K key, V value) {
public V put(@Nullable K key, @Nullable V value) {
return put(key, value, true);
}
@Override
@Nullable
public V putIfAbsent(K key, V value) {
public V putIfAbsent(@Nullable K key, @Nullable V value) {
return put(key, value, false);
}
@Nullable
private V put(final K key, final V value, final boolean overwriteExisting) {
private V put(@Nullable final K key, @Nullable final V value, final boolean overwriteExisting) {
return doTask(key, new Task<V>(TaskOption.RESTRUCTURE_BEFORE, TaskOption.RESIZE) {
@Override
@Nullable
protected V execute(@Nullable Reference<K, V> reference, @Nullable Entry<K, V> entry, @Nullable Entries entries) {
if (entry != null) {
V previousValue = entry.getValue();
V oldValue = entry.getValue();
if (overwriteExisting) {
entry.setValue(value);
}
return previousValue;
return oldValue;
}
Assert.state(entries != null, "No entries segment");
entries.add(value);
@ -342,9 +354,9 @@ public class ConcurrentReferenceHashMap<K, V> extends AbstractMap<K, V> implemen
@Nullable
protected V execute(@Nullable Reference<K, V> reference, @Nullable Entry<K, V> entry) {
if (entry != null) {
V previousValue = entry.getValue();
V oldValue = entry.getValue();
entry.setValue(value);
return previousValue;
return oldValue;
}
return null;
}
@ -389,7 +401,7 @@ public class ConcurrentReferenceHashMap<K, V> extends AbstractMap<K, V> implemen
}
@Nullable
private <T> T doTask(Object key, Task<T> task) {
private <T> T doTask(@Nullable Object key, Task<T> task) {
int hash = getHash(key);
return getSegmentForHash(hash).doTask(hash, key, task);
}
@ -488,7 +500,7 @@ public class ConcurrentReferenceHashMap<K, V> extends AbstractMap<K, V> implemen
* @return the result of the operation
*/
@Nullable
public <T> T doTask(final int hash, final Object key, final Task<T> task) {
public <T> T doTask(final int hash, @Nullable final Object key, final Task<T> task) {
boolean resize = task.hasOption(TaskOption.RESIZE);
if (task.hasOption(TaskOption.RESTRUCTURE_BEFORE)) {
restructureIfNecessary(resize);
@ -504,7 +516,7 @@ public class ConcurrentReferenceHashMap<K, V> extends AbstractMap<K, V> implemen
Entry<K, V> entry = (reference != null ? reference.get() : null);
Entries entries = new Entries() {
@Override
public void add(V value) {
public void add(@Nullable V value) {
@SuppressWarnings("unchecked")
Entry<K, V> newEntry = new Entry<>((K) key, value);
Reference<K, V> newReference = Segment.this.referenceManager.createReference(newEntry, hash, head);
@ -617,7 +629,7 @@ public class ConcurrentReferenceHashMap<K, V> extends AbstractMap<K, V> implemen
Entry<K, V> entry = currRef.get();
if (entry != null) {
K entryKey = entry.getKey();
if (entryKey == key || entryKey.equals(key)) {
if (ObjectUtils.nullSafeEquals(entryKey, key)) {
return currRef;
}
}
@ -688,27 +700,32 @@ public class ConcurrentReferenceHashMap<K, V> extends AbstractMap<K, V> implemen
*/
protected static final class Entry<K, V> implements Map.Entry<K, V> {
@Nullable
private final K key;
@Nullable
private volatile V value;
public Entry(K key, V value) {
public Entry(@Nullable K key, @Nullable V value) {
this.key = key;
this.value = value;
}
@Override
@Nullable
public K getKey() {
return this.key;
}
@Override
@Nullable
public V getValue() {
return this.value;
}
@Override
public V setValue(V value) {
@Nullable
public V setValue(@Nullable V value) {
V previous = this.value;
this.value = value;
return previous;
@ -800,7 +817,7 @@ public class ConcurrentReferenceHashMap<K, V> extends AbstractMap<K, V> implemen
* Add a new entry with the specified value.
* @param value the value to add
*/
public abstract void add(V value);
public abstract void add(@Nullable V value);
}

123
spring-core/src/test/java/org/springframework/util/ConcurrentReferenceHashMapTests.java
View File

@ -1,5 +1,5 @@
/*
* Copyright 2002-2016 the original author or authors.
* Copyright 2002-2018 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@ -61,7 +61,7 @@ public class ConcurrentReferenceHashMapTests {
@Test
public void shouldCreateWithDefaults() throws Exception {
public void shouldCreateWithDefaults() {
ConcurrentReferenceHashMap<Integer, String> map = new ConcurrentReferenceHashMap<>();
assertThat(map.getSegmentsSize(), is(16));
assertThat(map.getSegment(0).getSize(), is(1));
@ -69,7 +69,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldCreateWithInitialCapacity() throws Exception {
public void shouldCreateWithInitialCapacity() {
ConcurrentReferenceHashMap<Integer, String> map = new ConcurrentReferenceHashMap<>(32);
assertThat(map.getSegmentsSize(), is(16));
assertThat(map.getSegment(0).getSize(), is(2));
@ -77,7 +77,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldCreateWithInitialCapacityAndLoadFactor() throws Exception {
public void shouldCreateWithInitialCapacityAndLoadFactor() {
ConcurrentReferenceHashMap<Integer, String> map = new ConcurrentReferenceHashMap<>(32, 0.5f);
assertThat(map.getSegmentsSize(), is(16));
assertThat(map.getSegment(0).getSize(), is(2));
@ -85,7 +85,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldCreateWithInitialCapacityAndConcurrenyLevel() throws Exception {
public void shouldCreateWithInitialCapacityAndConcurrenyLevel() {
ConcurrentReferenceHashMap<Integer, String> map = new ConcurrentReferenceHashMap<>(16, 2);
assertThat(map.getSegmentsSize(), is(2));
assertThat(map.getSegment(0).getSize(), is(8));
@ -93,7 +93,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldCreateFullyCustom() throws Exception {
public void shouldCreateFullyCustom() {
ConcurrentReferenceHashMap<Integer, String> map = new ConcurrentReferenceHashMap<>(5, 0.5f, 3);
// concurrencyLevel of 3 ends up as 4 (nearest power of 2)
assertThat(map.getSegmentsSize(), is(4));
@ -103,7 +103,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldNeedNonNegativeInitialCapacity() throws Exception {
public void shouldNeedNonNegativeInitialCapacity() {
new ConcurrentReferenceHashMap<Integer, String>(0, 1);
this.thrown.expect(IllegalArgumentException.class);
this.thrown.expectMessage("Initial capacity must not be negative");
@ -111,7 +111,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldNeedPositiveLoadFactor() throws Exception {
public void shouldNeedPositiveLoadFactor() {
new ConcurrentReferenceHashMap<Integer, String>(0, 0.1f, 1);
this.thrown.expect(IllegalArgumentException.class);
this.thrown.expectMessage("Load factor must be positive");
@ -119,7 +119,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldNeedPositiveConcurrencyLevel() throws Exception {
public void shouldNeedPositiveConcurrencyLevel() {
new ConcurrentReferenceHashMap<Integer, String>(1, 1);
this.thrown.expect(IllegalArgumentException.class);
this.thrown.expectMessage("Concurrency level must be positive");
@ -127,7 +127,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldPutAndGet() throws Exception {
public void shouldPutAndGet() {
// NOTE we are using mock references so we don't need to worry about GC
assertThat(this.map.size(), is(0));
this.map.put(123, "123");
@ -140,32 +140,40 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldReplaceOnDoublePut() throws Exception {
public void shouldReplaceOnDoublePut() {
this.map.put(123, "321");
this.map.put(123, "123");
assertThat(this.map.get(123), is("123"));
}
@Test
public void shouldPutNullKey() throws Exception {
public void shouldPutNullKey() {
assertThat(this.map.get(null), is(nullValue()));
assertThat(this.map.getOrDefault(null, "456"), is("456"));
this.map.put(null, "123");
assertThat(this.map.get(null), is("123"));
assertThat(this.map.getOrDefault(null, "456"), is("123"));
}
@Test
public void shouldPutNullValue() throws Exception {
public void shouldPutNullValue() {
assertThat(this.map.get(123), is(nullValue()));
assertThat(this.map.getOrDefault(123, "456"), is("456"));
this.map.put(123, "321");
assertThat(this.map.get(123), is("321"));
assertThat(this.map.getOrDefault(123, "456"), is("321"));
this.map.put(123, null);
assertThat(this.map.get(123), is(nullValue()));
assertThat(this.map.getOrDefault(123, "456"), is(nullValue()));
}
@Test
public void shouldGetWithNoItems() throws Exception {
public void shouldGetWithNoItems() {
assertThat(this.map.get(123), is(nullValue()));
}
@Test
public void shouldApplySupplimentalHash() throws Exception {
public void shouldApplySupplimentalHash() {
Integer key = 123;
this.map.put(key, "123");
assertThat(this.map.getSupplimentalHash(), is(not(key.hashCode())));
@ -173,7 +181,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldGetFollowingNexts() throws Exception {
public void shouldGetFollowingNexts() {
// Use loadFactor to disable resize
this.map = new TestWeakConcurrentCache<>(1, 10.0f, 1);
this.map.put(1, "1");
@ -187,7 +195,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldResize() throws Exception {
public void shouldResize() {
this.map = new TestWeakConcurrentCache<>(1, 0.75f, 1);
this.map.put(1, "1");
assertThat(this.map.getSegment(0).getSize(), is(1));
@ -217,7 +225,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldPurgeOnGet() throws Exception {
public void shouldPurgeOnGet() {
this.map = new TestWeakConcurrentCache<>(1, 0.75f, 1);
for (int i = 1; i <= 5; i++) {
this.map.put(i, String.valueOf(i));
@ -232,7 +240,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldPergeOnPut() throws Exception {
public void shouldPergeOnPut() {
this.map = new TestWeakConcurrentCache<>(1, 0.75f, 1);
for (int i = 1; i <= 5; i++) {
this.map.put(i, String.valueOf(i));
@ -248,28 +256,28 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldPutIfAbsent() throws Exception {
public void shouldPutIfAbsent() {
assertThat(this.map.putIfAbsent(123, "123"), is(nullValue()));
assertThat(this.map.putIfAbsent(123, "123b"), is("123"));
assertThat(this.map.get(123), is("123"));
}
@Test
public void shouldPutIfAbsentWithNullValue() throws Exception {
public void shouldPutIfAbsentWithNullValue() {
assertThat(this.map.putIfAbsent(123, null), is(nullValue()));
assertThat(this.map.putIfAbsent(123, "123"), is(nullValue()));
assertThat(this.map.get(123), is(nullValue()));
}
@Test
public void shouldPutIfAbsentWithNullKey() throws Exception {
public void shouldPutIfAbsentWithNullKey() {
assertThat(this.map.putIfAbsent(null, "123"), is(nullValue()));
assertThat(this.map.putIfAbsent(null, "123b"), is("123"));
assertThat(this.map.get(null), is("123"));
}
@Test
public void shouldRemoveKeyAndValue() throws Exception {
public void shouldRemoveKeyAndValue() {
this.map.put(123, "123");
assertThat(this.map.remove(123, "456"), is(false));
assertThat(this.map.get(123), is("123"));
@ -279,7 +287,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldRemoveKeyAndValueWithExistingNull() throws Exception {
public void shouldRemoveKeyAndValueWithExistingNull() {
this.map.put(123, null);
assertThat(this.map.remove(123, "456"), is(false));
assertThat(this.map.get(123), is(nullValue()));
@ -289,7 +297,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldReplaceOldValueWithNewValue() throws Exception {
public void shouldReplaceOldValueWithNewValue() {
this.map.put(123, "123");
assertThat(this.map.replace(123, "456", "789"), is(false));
assertThat(this.map.get(123), is("123"));
@ -298,7 +306,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldReplaceOldNullValueWithNewValue() throws Exception {
public void shouldReplaceOldNullValueWithNewValue() {
this.map.put(123, null);
assertThat(this.map.replace(123, "456", "789"), is(false));
assertThat(this.map.get(123), is(nullValue()));
@ -307,21 +315,21 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldReplaceValue() throws Exception {
public void shouldReplaceValue() {
this.map.put(123, "123");
assertThat(this.map.replace(123, "456"), is("123"));
assertThat(this.map.get(123), is("456"));
}
@Test
public void shouldReplaceNullValue() throws Exception {
public void shouldReplaceNullValue() {
this.map.put(123, null);
assertThat(this.map.replace(123, "456"), is(nullValue()));
assertThat(this.map.get(123), is("456"));
}
@Test
public void shouldGetSize() throws Exception {
public void shouldGetSize() {
assertThat(this.map.size(), is(0));
this.map.put(123, "123");
this.map.put(123, null);
@ -330,7 +338,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldSupportIsEmpty() throws Exception {
public void shouldSupportIsEmpty() {
assertThat(this.map.isEmpty(), is(true));
this.map.put(123, "123");
this.map.put(123, null);
@ -339,7 +347,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldContainKey() throws Exception {
public void shouldContainKey() {
assertThat(this.map.containsKey(123), is(false));
assertThat(this.map.containsKey(456), is(false));
this.map.put(123, "123");
@ -349,7 +357,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldContainValue() throws Exception {
public void shouldContainValue() {
assertThat(this.map.containsValue("123"), is(false));
assertThat(this.map.containsValue(null), is(false));
this.map.put(123, "123");
@ -359,7 +367,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldRemoveWhenKeyIsInMap() throws Exception {
public void shouldRemoveWhenKeyIsInMap() {
this.map.put(123, null);
this.map.put(456, "456");
this.map.put(null, "789");
@ -370,14 +378,14 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldRemoveWhenKeyIsNotInMap() throws Exception {
public void shouldRemoveWhenKeyIsNotInMap() {
assertThat(this.map.remove(123), is(nullValue()));
assertThat(this.map.remove(null), is(nullValue()));
assertThat(this.map.isEmpty(), is(true));
}
@Test
public void shouldPutAll() throws Exception {
public void shouldPutAll() {
Map<Integer, String> m = new HashMap<>();
m.put(123, "123");
m.put(456, null);
@ -390,7 +398,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldClear() throws Exception {
public void shouldClear() {
this.map.put(123, "123");
this.map.put(456, null);
this.map.put(null, "789");
@ -402,7 +410,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldGetKeySet() throws Exception {
public void shouldGetKeySet() {
this.map.put(123, "123");
this.map.put(456, null);
this.map.put(null, "789");
@ -414,7 +422,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldGetValues() throws Exception {
public void shouldGetValues() {
this.map.put(123, "123");
this.map.put(456, null);
this.map.put(null, "789");
@ -423,13 +431,13 @@ public class ConcurrentReferenceHashMapTests {
expected.add("123");
expected.add(null);
expected.add("789");
Collections.sort(actual, NULL_SAFE_STRING_SORT);
Collections.sort(expected, NULL_SAFE_STRING_SORT);
actual.sort(NULL_SAFE_STRING_SORT);
expected.sort(NULL_SAFE_STRING_SORT);
assertThat(actual, is(expected));
}
@Test
public void shouldGetEntrySet() throws Exception {
public void shouldGetEntrySet() {
this.map.put(123, "123");
this.map.put(456, null);
this.map.put(null, "789");
@ -441,7 +449,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldGetEntrySetFollowingNext() throws Exception {
public void shouldGetEntrySetFollowingNext() {
// Use loadFactor to disable resize
this.map = new TestWeakConcurrentCache<>(1, 10.0f, 1);
this.map.put(1, "1");
@ -455,7 +463,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldRemoveViaEntrySet() throws Exception {
public void shouldRemoveViaEntrySet() {
this.map.put(1, "1");
this.map.put(2, "2");
this.map.put(3, "3");
@ -470,7 +478,7 @@ public class ConcurrentReferenceHashMapTests {
}
@Test
public void shouldSetViaEntrySet() throws Exception {
public void shouldSetViaEntrySet() {
this.map.put(1, "1");
this.map.put(2, "2");
this.map.put(3, "3");
@ -485,36 +493,21 @@ public class ConcurrentReferenceHashMapTests {
@Test
@Ignore("Intended for use during development only")
public void shouldBeFasterThanSynchronizedMap() throws Exception {
public void shouldBeFasterThanSynchronizedMap() throws InterruptedException {
Map<Integer, WeakReference<String>> synchronizedMap = Collections.synchronizedMap(new WeakHashMap<Integer, WeakReference<String>>());
StopWatch mapTime = timeMultiThreaded("SynchronizedMap", synchronizedMap,
new ValueFactory<WeakReference<String>>() {
@Override
public WeakReference<String> newValue(int v) {
return new WeakReference<>(String.valueOf(v));
}
});
StopWatch mapTime = timeMultiThreaded("SynchronizedMap", synchronizedMap, v -> new WeakReference<>(String.valueOf(v)));
System.out.println(mapTime.prettyPrint());
this.map.setDisableTestHooks(true);
StopWatch cacheTime = timeMultiThreaded("WeakConcurrentCache", this.map,
new ValueFactory<String>() {
@Override
public String newValue(int v) {
return String.valueOf(v);
}
});
StopWatch cacheTime = timeMultiThreaded("WeakConcurrentCache", this.map, String::valueOf);
System.out.println(cacheTime.prettyPrint());
// We should be at least 4 time faster
assertThat(cacheTime.getTotalTimeSeconds(),
is(lessThan(mapTime.getTotalTimeSeconds() / 4.0)));
assertThat(cacheTime.getTotalTimeSeconds(), is(lessThan(mapTime.getTotalTimeSeconds() / 4.0)));
}
@Test
public void shouldSupportNullReference() throws Exception {
public void shouldSupportNullReference() {
// GC could happen during restructure so we must be able to create a reference for a null entry
map.createReferenceManager().createReference(null, 1234, null);
}
@ -558,7 +551,7 @@ public class ConcurrentReferenceHashMapTests {
}
private static interface ValueFactory<V> {
private interface ValueFactory<V> {
V newValue(int k);
}