Improve thread safety in property source cache
Update `SpringIterableConfigurationPropertySource` so that they cache and cache key are not stored in different fields. Prior to this commit it was possible that the an incorrect cache could be returned from because the key and cache were out of sync. This commit also allows more lenient handling of ConcurrentModification exceptions if they are thrown during cache retrieval. Closes gh-17017 See gh-17013
This commit is contained in:
@@ -18,6 +18,7 @@ package org.springframework.boot.context.properties.source;
|
||||
|
||||
import java.util.ArrayList;
|
||||
import java.util.Collections;
|
||||
import java.util.ConcurrentModificationException;
|
||||
import java.util.HashSet;
|
||||
import java.util.Iterator;
|
||||
import java.util.List;
|
||||
@@ -44,8 +45,6 @@ import org.springframework.util.ObjectUtils;
|
||||
class SpringIterableConfigurationPropertySource extends SpringConfigurationPropertySource
|
||||
implements IterableConfigurationPropertySource {
|
||||
|
||||
private volatile Object cacheKey;
|
||||
|
||||
private volatile Cache cache;
|
||||
|
||||
SpringIterableConfigurationPropertySource(EnumerablePropertySource<?> propertySource,
|
||||
@@ -131,16 +130,23 @@ class SpringIterableConfigurationPropertySource extends SpringConfigurationPrope
|
||||
}
|
||||
|
||||
private Cache getCache() {
|
||||
CacheKey cacheKey = CacheKey.get(getPropertySource());
|
||||
if (cacheKey == null) {
|
||||
CacheKey key = CacheKey.get(getPropertySource());
|
||||
if (key == null) {
|
||||
return null;
|
||||
}
|
||||
if (ObjectUtils.nullSafeEquals(cacheKey, this.cacheKey)) {
|
||||
return this.cache;
|
||||
Cache cache = this.cache;
|
||||
try {
|
||||
if (cache != null && cache.hasKeyEqualTo(key)) {
|
||||
return cache;
|
||||
}
|
||||
cache = new Cache(key.copy());
|
||||
this.cache = cache;
|
||||
return cache;
|
||||
}
|
||||
catch (ConcurrentModificationException ex) {
|
||||
// Not fatal at this point, we can continue without a cache
|
||||
return null;
|
||||
}
|
||||
this.cache = new Cache();
|
||||
this.cacheKey = cacheKey.copy();
|
||||
return this.cache;
|
||||
}
|
||||
|
||||
@Override
|
||||
@@ -150,10 +156,20 @@ class SpringIterableConfigurationPropertySource extends SpringConfigurationPrope
|
||||
|
||||
private static class Cache {
|
||||
|
||||
private final CacheKey key;
|
||||
|
||||
private List<ConfigurationPropertyName> names;
|
||||
|
||||
private PropertyMapping[] mappings;
|
||||
|
||||
Cache(CacheKey key) {
|
||||
this.key = key;
|
||||
}
|
||||
|
||||
public boolean hasKeyEqualTo(CacheKey key) {
|
||||
return this.key.equals(key);
|
||||
}
|
||||
|
||||
public List<ConfigurationPropertyName> getNames() {
|
||||
return this.names;
|
||||
}
|
||||
|
||||
@@ -16,8 +16,12 @@
|
||||
|
||||
package org.springframework.boot.context.properties.source;
|
||||
|
||||
import java.util.ConcurrentModificationException;
|
||||
import java.util.Iterator;
|
||||
import java.util.LinkedHashMap;
|
||||
import java.util.LinkedHashSet;
|
||||
import java.util.Map;
|
||||
import java.util.Set;
|
||||
|
||||
import org.junit.Test;
|
||||
|
||||
@@ -169,6 +173,21 @@ public class SpringIterableConfigurationPropertySourceTests {
|
||||
assertThat(adapter.stream().count()).isEqualTo(3);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void concurrentModificationExceptionInvalidatesCache() {
|
||||
// gh-17013
|
||||
ConcurrentModificationThrowingMap<String, Object> map = new ConcurrentModificationThrowingMap<>();
|
||||
map.put("key1", "value1");
|
||||
map.put("key2", "value2");
|
||||
EnumerablePropertySource<?> source = new MapPropertySource("test", map);
|
||||
SpringIterableConfigurationPropertySource adapter = new SpringIterableConfigurationPropertySource(
|
||||
source, DefaultPropertyMapper.INSTANCE);
|
||||
assertThat(adapter.stream().count()).isEqualTo(2);
|
||||
map.setThrowException(true);
|
||||
map.put("key3", "value3");
|
||||
assertThat(adapter.stream().count()).isEqualTo(3);
|
||||
}
|
||||
|
||||
/**
|
||||
* Test {@link PropertySource} that's also an {@link OriginLookup}.
|
||||
*/
|
||||
@@ -206,4 +225,37 @@ public class SpringIterableConfigurationPropertySourceTests {
|
||||
|
||||
}
|
||||
|
||||
private static class ConcurrentModificationThrowingMap<K, V>
|
||||
extends LinkedHashMap<K, V> {
|
||||
|
||||
private boolean throwException;
|
||||
|
||||
public void setThrowException(boolean throwException) {
|
||||
this.throwException = throwException;
|
||||
}
|
||||
|
||||
@Override
|
||||
public Set<K> keySet() {
|
||||
return new KeySet(super.keySet());
|
||||
}
|
||||
|
||||
private class KeySet extends LinkedHashSet<K> {
|
||||
|
||||
KeySet(Set<K> keySet) {
|
||||
super(keySet);
|
||||
}
|
||||
|
||||
@Override
|
||||
public Iterator<K> iterator() {
|
||||
if (ConcurrentModificationThrowingMap.this.throwException) {
|
||||
ConcurrentModificationThrowingMap.this.throwException = false;
|
||||
throw new ConcurrentModificationException();
|
||||
}
|
||||
return super.iterator();
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user