From 356ef45e99c9d1e409f3d04330c44e43d2b45811 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Wed, 14 Mar 2018 17:17:04 +0100 Subject: [PATCH] ConcurrentReferenceHashMap properly handles getOrDefault for null values Issue: SPR-16584 --- .../util/ConcurrentReferenceHashMap.java | 59 ++++++--- .../util/ConcurrentReferenceHashMapTests.java | 123 +++++++++--------- 2 files changed, 96 insertions(+), 86 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 2804b84058..e0502f7ce6 100644 --- a/spring-core/src/main/java/org/springframework/util/ConcurrentReferenceHashMap.java +++ b/spring-core/src/main/java/org/springframework/util/ConcurrentReferenceHashMap.java @@ -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 the key type * @param the value type @@ -226,19 +227,30 @@ public class ConcurrentReferenceHashMap extends AbstractMap implemen @Override @Nullable - public V get(Object key) { - Reference reference = getReference(key, Restructure.WHEN_NECESSARY); - Entry entry = (reference != null ? reference.get() : null); + public V get(@Nullable Object key) { + Entry entry = getEntryIfAvailable(key); return (entry != null ? entry.getValue() : null); } @Override - public boolean containsKey(Object key) { - Reference reference = getReference(key, Restructure.WHEN_NECESSARY); - Entry entry = (reference != null ? reference.get() : null); + @Nullable + public V getOrDefault(@Nullable Object key, @Nullable V defaultValue) { + Entry entry = getEntryIfAvailable(key); + return (entry != null ? entry.getValue() : defaultValue); + } + + @Override + public boolean containsKey(@Nullable Object key) { + Entry entry = getEntryIfAvailable(key); return (entry != null && ObjectUtils.nullSafeEquals(entry.getKey(), key)); } + @Nullable + private Entry getEntryIfAvailable(@Nullable Object key) { + Reference 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 extends AbstractMap 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(TaskOption.RESTRUCTURE_BEFORE, TaskOption.RESIZE) { @Override @Nullable protected V execute(@Nullable Reference reference, @Nullable Entry 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 extends AbstractMap implemen @Nullable protected V execute(@Nullable Reference reference, @Nullable Entry 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 extends AbstractMap implemen } @Nullable - private T doTask(Object key, Task task) { + private T doTask(@Nullable Object key, Task task) { int hash = getHash(key); return getSegmentForHash(hash).doTask(hash, key, task); } @@ -488,7 +500,7 @@ public class ConcurrentReferenceHashMap extends AbstractMap implemen * @return the result of the operation */ @Nullable - public T doTask(final int hash, final Object key, final Task task) { + public T doTask(final int hash, @Nullable final Object key, final Task task) { boolean resize = task.hasOption(TaskOption.RESIZE); if (task.hasOption(TaskOption.RESTRUCTURE_BEFORE)) { restructureIfNecessary(resize); @@ -504,7 +516,7 @@ public class ConcurrentReferenceHashMap extends AbstractMap implemen Entry 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 newEntry = new Entry<>((K) key, value); Reference newReference = Segment.this.referenceManager.createReference(newEntry, hash, head); @@ -617,7 +629,7 @@ public class ConcurrentReferenceHashMap extends AbstractMap implemen Entry 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 extends AbstractMap implemen */ protected static final class Entry implements Map.Entry { + @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 extends AbstractMap 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); } 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 00d59848b7..c4e81bb691 100644 --- a/spring-core/src/test/java/org/springframework/util/ConcurrentReferenceHashMapTests.java +++ b/spring-core/src/test/java/org/springframework/util/ConcurrentReferenceHashMapTests.java @@ -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 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 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 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 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 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(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(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(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 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> synchronizedMap = Collections.synchronizedMap(new WeakHashMap>()); - StopWatch mapTime = timeMultiThreaded("SynchronizedMap", synchronizedMap, - new ValueFactory>() { - - @Override - public WeakReference 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() { - - @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 { + private interface ValueFactory { V newValue(int k); } -- GitLab