Repository: incubator-brooklyn Updated Branches: refs/heads/master 04fc801d0 -> bf07fe1f0
make ConfigBag thread-safe Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/5e34f950 Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/5e34f950 Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/5e34f950 Branch: refs/heads/master Commit: 5e34f950f1d6a7875224112b60ae2de810b18feb Parents: 3ce25c9 Author: Alex Heneveld <[email protected]> Authored: Thu Apr 9 12:26:18 2015 +0100 Committer: Alex Heneveld <[email protected]> Committed: Thu Apr 9 12:27:07 2015 +0100 ---------------------------------------------------------------------- .../java/brooklyn/util/config/ConfigBag.java | 130 +++++++++---- .../brooklyn/util/config/ConfigBagTest.java | 191 +++++++++++++++++++ 2 files changed, 287 insertions(+), 34 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/5e34f950/core/src/main/java/brooklyn/util/config/ConfigBag.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/util/config/ConfigBag.java b/core/src/main/java/brooklyn/util/config/ConfigBag.java index c57098b..220ec52 100644 --- a/core/src/main/java/brooklyn/util/config/ConfigBag.java +++ b/core/src/main/java/brooklyn/util/config/ConfigBag.java @@ -20,7 +20,7 @@ package brooklyn.util.config; import static com.google.common.base.Preconditions.checkNotNull; -import java.util.Collections; +import java.util.ConcurrentModificationException; import java.util.LinkedHashMap; import java.util.Map; @@ -37,6 +37,7 @@ import brooklyn.util.flags.TypeCoercions; import brooklyn.util.guava.Maybe; import brooklyn.util.javalang.JavaClassNames; +import com.google.common.annotations.Beta; import com.google.common.base.Objects; import com.google.common.collect.Sets; @@ -47,7 +48,9 @@ import com.google.common.collect.Sets; * It is recommended to use {@link ConfigKey} instances to access, * although in some cases (such as setting fields from flags, or copying a map) * it may be necessary to mark things as used, or put, when only a string key is available. - * + * <p> + * This bag is order-preserving and thread-safe except where otherwise indicated. + * <p> * @author alex */ public class ConfigBag { @@ -71,6 +74,9 @@ public class ConfigBag { /** * Creates an instance that is backed by a "live map" (e.g. storage in a datagrid). + * The order-preserving nature of this class is only guaranteed if the + * provided storage has those properties. External modifications to the store can cause + * {@link ConcurrentModificationException} to be thrown, here or elsewhere. */ public static ConfigBag newLiveInstance(Map<String,Object> storage) { return new ConfigBag(checkNotNull(storage, "storage map must be specified")); @@ -96,14 +102,42 @@ public class ConfigBag { * (note: this applies even for values which are overridden and the overridden value is used); * however subsequent uses in the original set will not be marked here */ - public static ConfigBag newInstanceExtending(final ConfigBag configBag, Map<?,?> flags) { + @Beta + public static ConfigBag newInstanceExtending(final ConfigBag parentBag) { + return new ConfigBagExtendingParent(parentBag).copy(parentBag); + } + + /** @see #newInstanceExtending(ConfigBag) */ + public static class ConfigBagExtendingParent extends ConfigBag { + ConfigBag parentBag; + private ConfigBagExtendingParent(ConfigBag parentBag) { + this.parentBag = parentBag; + } + @Override + public void markUsed(String key) { + super.markUsed(key); + if (parentBag!=null) + parentBag.markUsed(key); + } + } + + /** As {@link #newInstanceExtending(ConfigBag)} but also putting the supplied values. */ + @Beta + public static ConfigBag newInstanceExtending(final ConfigBag configBag, Map<?,?> optionalAdditionalValues) { + return newInstanceExtending(configBag).putAll(optionalAdditionalValues); + } + + /** @deprecated since 0.7.0, not used; kept only for rebind compatibility where the inner class is used + * (now replaced by a static class above) */ + @Beta @Deprecated + public static ConfigBag newInstanceWithInnerClass(final ConfigBag configBag, Map<?,?> optionalAdditionalValues) { return new ConfigBag() { @Override public void markUsed(String key) { super.markUsed(key); configBag.markUsed(key); } - }.copy(configBag).putAll(flags); + }.copy(configBag).putAll(optionalAdditionalValues); } public ConfigBag() { @@ -132,12 +166,12 @@ public class ConfigBag { /** current values for all entries * @return non-modifiable map of strings to object */ - public Map<String,Object> getAllConfig() { - return Collections.unmodifiableMap(config); + public synchronized Map<String,Object> getAllConfig() { + return MutableMap.copyOf(config).asUnmodifiable(); } /** current values for all entries in a map where the keys are converted to {@link ConfigKey} instances */ - public Map<ConfigKey<?>, ?> getAllConfigAsConfigKeyMap() { + public synchronized Map<ConfigKey<?>, ?> getAllConfigAsConfigKeyMap() { Map<ConfigKey<?>,Object> result = MutableMap.of(); for (Map.Entry<String,Object> entry: config.entrySet()) { result.put(ConfigKeys.newConfigKey(Object.class, entry.getKey()), entry.getValue()); @@ -145,15 +179,18 @@ public class ConfigBag { return result; } - /** internal map containing the current values for all entries; - * for use where the caller wants to modify this directly and knows it is safe to do so */ + /** Returns the internal map containing the current values for all entries; + * for use where the caller wants to modify this directly and knows it is safe to do so + * <p> + * Accesses to the returned map must be synchronized on this bag if the + * thread-safe behaviour is required. */ public Map<String,Object> getAllConfigMutable() { if (live) { // TODO sealed no longer works as before, because `config` is the backing storage map. // Therefore returning it is dangerous! Even if we were to replace our field with an immutable copy, // the underlying datagrid's map would still be modifiable. We need a way to switch the returned // value's behaviour to sealable (i.e. wrapping the returned map). - return (sealed) ? Collections.unmodifiableMap(config) : config; + return (sealed) ? MutableMap.copyOf(config).asUnmodifiable() : config; } else { return config; } @@ -161,12 +198,15 @@ public class ConfigBag { /** current values for all entries which have not yet been used * @return non-modifiable map of strings to object */ - public Map<String,Object> getUnusedConfig() { - return Collections.unmodifiableMap(unusedConfig); + public synchronized Map<String,Object> getUnusedConfig() { + return MutableMap.copyOf(unusedConfig).asUnmodifiable(); } - /** internal map containing the current values for all entries which have not yet been used; - * for use where the caller wants to modify this directly and knows it is safe to do so */ + /** Returns the internal map containing the current values for all entries which have not yet been used; + * for use where the caller wants to modify this directly and knows it is safe to do so + * <p> + * Accesses to the returned map must be synchronized on this bag if the + * thread-safe behaviour is required. */ public Map<String,Object> getUnusedConfigMutable() { return unusedConfig; } @@ -191,7 +231,7 @@ public class ConfigBag { return putIfAbsent(MutableMap.of(key, value)); } - public ConfigBag putIfAbsent(Map<?, ?> propertiesToSet) { + public synchronized ConfigBag putIfAbsent(Map<?, ?> propertiesToSet) { if (propertiesToSet==null) return this; for (Map.Entry<?, ?> entry: propertiesToSet.entrySet()) { @@ -242,7 +282,7 @@ public class ConfigBag { return this; } - protected void putAsStringKey(Object key, Object value) { + protected synchronized void putAsStringKey(Object key, Object value) { if (key instanceof HasConfigKey<?>) key = ((HasConfigKey<?>)key).getConfigKey(); if (key instanceof ConfigKey<?>) key = ((ConfigKey<?>)key).getName(); if (key instanceof String) { @@ -262,7 +302,7 @@ public class ConfigBag { /** recommended to use {@link #put(ConfigKey, Object)} but there are times * (e.g. when copying a map) where we want to put a string key directly */ - public Object putStringKey(String key, Object value) { + public synchronized Object putStringKey(String key, Object value) { if (sealed) throw new IllegalStateException("Cannot insert "+key+"="+value+": this config bag has been sealed and is now immutable."); boolean isNew = !config.containsKey(key); @@ -285,14 +325,14 @@ public class ConfigBag { } public boolean containsKey(HasConfigKey<?> key) { - return config.containsKey(key.getConfigKey()); + return containsKey(key.getConfigKey()); } public boolean containsKey(ConfigKey<?> key) { - return config.containsKey(key.getName()); + return containsKey(key.getName()); } - public boolean containsKey(String key) { + public synchronized boolean containsKey(String key) { return config.containsKey(key); } @@ -318,7 +358,7 @@ public class ConfigBag { } /** returns the first key in the list for which a value is explicitly set, then defaulting to defaulting value of preferred key */ - public <T> T getFirst(ConfigKey<T> preferredKey, ConfigKey<T> ...otherCurrentKeysInOrderOfPreference) { + public synchronized <T> T getFirst(ConfigKey<T> preferredKey, ConfigKey<T> ...otherCurrentKeysInOrderOfPreference) { if (containsKey(preferredKey)) return get(preferredKey); for (ConfigKey<T> key: otherCurrentKeysInOrderOfPreference) { @@ -336,7 +376,7 @@ public class ConfigBag { /** returns the value for the first key in the list for which a value is set, * warning if any of the deprecated keys have a value which is different to that set on the first set current key * (including warning if a deprecated key has a value but no current key does) */ - public Object getWithDeprecation(ConfigKey<?>[] currentKeysInOrderOfPreference, ConfigKey<?> ...deprecatedKeys) { + public synchronized Object getWithDeprecation(ConfigKey<?>[] currentKeysInOrderOfPreference, ConfigKey<?> ...deprecatedKeys) { // Get preferred key (or null) ConfigKey<?> preferredKeyProvidingValue = null; Object result = null; @@ -399,10 +439,8 @@ public class ConfigBag { protected <T> T get(ConfigKey<T> key, boolean markUsed) { // TODO for now, no evaluation -- maps / closure content / other smart (self-extracting) keys are NOT supported // (need a clean way to inject that behaviour, as well as desired TypeCoercions) - if (config.containsKey(key.getName())) - return coerceFirstNonNullKeyValue(key, getStringKey(key.getName(), markUsed)); - - return coerceFirstNonNullKeyValue(key); + // this method, and the coercion, is not synchronized, nor does it need to be, because the "get" is synchronized. + return coerceFirstNonNullKeyValue(key, getStringKey(key.getName(), markUsed)); } /** returns the first non-null value to be the type indicated by the key, or the keys default value if no non-null values are supplied */ @@ -415,7 +453,7 @@ public class ConfigBag { protected Object getStringKey(String key, boolean markUsed) { return getStringKeyMaybe(key, markUsed).orNull(); } - protected Maybe<Object> getStringKeyMaybe(String key, boolean markUsed) { + protected synchronized Maybe<Object> getStringKeyMaybe(String key, boolean markUsed) { if (config.containsKey(key)) { if (markUsed) markUsed(key); return Maybe.of(config.get(key)); @@ -424,11 +462,11 @@ public class ConfigBag { } /** indicates that a string key in the config map has been accessed */ - public void markUsed(String key) { + public synchronized void markUsed(String key) { unusedConfig.remove(key); } - public void clear() { + public synchronized void clear() { if (sealed) throw new IllegalStateException("Cannot clear this config bag has been sealed and is now immutable."); config.clear(); @@ -440,7 +478,7 @@ public class ConfigBag { return this; } - public void remove(ConfigKey<?> key) { + public synchronized void remove(ConfigKey<?> key) { remove(key.getName()); } @@ -449,7 +487,7 @@ public class ConfigBag { return this; } - public void remove(String key) { + public synchronized void remove(String key) { if (sealed) throw new IllegalStateException("Cannot remove "+key+": this config bag has been sealed and is now immutable."); config.remove(key); @@ -457,6 +495,28 @@ public class ConfigBag { } public ConfigBag copy(ConfigBag other) { + // ensure locks are taken in a canonical order to prevent deadlock + if (other==null) { + synchronized (this) { + return copyWhileSynched(other); + } + } + if (System.identityHashCode(other) < System.identityHashCode(this)) { + synchronized (other) { + synchronized (this) { + return copyWhileSynched(other); + } + } + } else { + synchronized (this) { + synchronized (other) { + return copyWhileSynched(other); + } + } + } + } + + protected ConfigBag copyWhileSynched(ConfigBag other) { if (sealed) throw new IllegalStateException("Cannot copy "+other+" to "+this+": this config bag has been sealed and is now immutable."); putAll(other.getAllConfig()); @@ -465,11 +525,11 @@ public class ConfigBag { return this; } - public int size() { + public synchronized int size() { return config.size(); } - public boolean isEmpty() { + public synchronized boolean isEmpty() { return config.isEmpty(); } @@ -479,7 +539,7 @@ public class ConfigBag { return this; } - public boolean isUnused(ConfigKey<?> key) { + public synchronized boolean isUnused(ConfigKey<?> key) { return unusedConfig.containsKey(key.getName()); } @@ -499,6 +559,8 @@ public class ConfigBag { return this; } + // TODO why have both this and mutable + /** @see #getAllConfigMutable() */ public Map<String, Object> getAllConfigRaw() { return getAllConfigMutable(); } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/5e34f950/core/src/test/java/brooklyn/util/config/ConfigBagTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/brooklyn/util/config/ConfigBagTest.java b/core/src/test/java/brooklyn/util/config/ConfigBagTest.java new file mode 100644 index 0000000..93cf6ed --- /dev/null +++ b/core/src/test/java/brooklyn/util/config/ConfigBagTest.java @@ -0,0 +1,191 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package brooklyn.util.config; + +import static org.testng.Assert.assertEquals; + +import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicInteger; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.testng.Assert; +import org.testng.annotations.Test; + +import brooklyn.config.ConfigKey; +import brooklyn.entity.basic.ConfigKeys; +import brooklyn.util.collections.MutableList; +import brooklyn.util.collections.MutableMap; +import brooklyn.util.exceptions.Exceptions; +import brooklyn.util.time.Duration; + +public class ConfigBagTest { + + @SuppressWarnings("unused") + private static final Logger log = LoggerFactory.getLogger(ConfigBagTest.class); + + private static final ConfigKey<String> K1 = ConfigKeys.newStringConfigKey("k1"); + private static final ConfigKey<String> K2 = ConfigKeys.newStringConfigKey("k2"); + private static final ConfigKey<String> K3 = ConfigKeys.newStringConfigKey("k3"); + + @Test + public void testPutAndGet() { + ConfigBag bag = ConfigBag.newInstance(); + bag.put(K1, "v1"); + assertEquals(bag.get(K1), "v1"); + } + + @Test + public void testPutStringAndGet() { + ConfigBag bag = ConfigBag.newInstance(); + bag.putAsStringKey(K1.getName(), "v1"); + assertEquals(bag.get(K1), "v1"); + } + + @Test + public void testUnused() { + ConfigBag bag = ConfigBag.newInstance(); + bag.put(K1, "v1"); + bag.put(K2, "v2a"); + assertEquals(bag.get(K1), "v1"); + assertEquals(bag.getUnusedConfig().size(), 1); + assertEquals(bag.peek(K2), "v2a"); + assertEquals(bag.getUnusedConfig().size(), 1); + assertEquals(bag.get(K2), "v2a"); + Assert.assertTrue(bag.getUnusedConfig().isEmpty()); + } + + @Test + public void testOrder() { + ConfigBag bag = ConfigBag.newInstance(); + bag.put(K1, "v1"); + bag.put(K2, "v2"); + bag.put(K3, "v3"); + Assert.assertEquals(MutableList.copyOf(bag.getAllConfig().keySet()), MutableList.of(K1.getName(), K2.getName(), K3.getName())); + Assert.assertEquals(MutableList.copyOf(bag.getAllConfig().values()), MutableList.of("v1", "v2", "v3")); + } + + @Test + public void testCopyOverwriteAndGet() { + ConfigBag bag1 = ConfigBag.newInstance(); + bag1.put(K1, "v1"); + bag1.put(K2, "v2a"); + bag1.put(K3, "v3"); + assertEquals(bag1.get(K1), "v1"); + + ConfigBag bag2 = ConfigBag.newInstanceCopying(bag1).putAll(MutableMap.of(K2, "v2b")); + assertEquals(bag1.getUnusedConfig().size(), 2); + assertEquals(bag2.getUnusedConfig().size(), 2); + + assertEquals(bag2.get(K1), "v1"); + assertEquals(bag1.get(K2), "v2a"); + assertEquals(bag1.getUnusedConfig().size(), 1); + assertEquals(bag2.getUnusedConfig().size(), 2); + + assertEquals(bag2.get(K2), "v2b"); + assertEquals(bag2.getUnusedConfig().size(), 1); + + assertEquals(bag2.get(K3), "v3"); + assertEquals(bag2.getUnusedConfig().size(), 0); + assertEquals(bag1.getUnusedConfig().size(), 1); + } + + @Test + public void testCopyExtendingAndGet() { + ConfigBag bag1 = ConfigBag.newInstance(); + bag1.put(K1, "v1"); + bag1.put(K2, "v2a"); + bag1.put(K3, "v3"); + assertEquals(bag1.get(K1), "v1"); + + ConfigBag bag2 = ConfigBag.newInstanceExtending(bag1, null).putAll(MutableMap.of(K2, "v2b")); + assertEquals(bag1.getUnusedConfig().size(), 2); + assertEquals(bag2.getUnusedConfig().size(), 2, "unused are: "+bag2.getUnusedConfig()); + + assertEquals(bag2.get(K1), "v1"); + assertEquals(bag1.get(K2), "v2a"); + assertEquals(bag1.getUnusedConfig().size(), 1); + assertEquals(bag2.getUnusedConfig().size(), 2); + + assertEquals(bag2.get(K2), "v2b"); + assertEquals(bag2.getUnusedConfig().size(), 1); + + assertEquals(bag2.get(K3), "v3"); + assertEquals(bag2.getUnusedConfig().size(), 0); + // when extended, the difference is that parent is also marked + assertEquals(bag1.getUnusedConfig().size(), 0); + } + + @Test + public void testConcurrent() throws InterruptedException { + ConfigBag bag = ConfigBag.newInstance(); + bag.put(K1, "v1"); + bag.put(K2, "v2"); + bag.put(K3, "v3"); + runConcurrentTest(bag, 10, Duration.millis(50)); + } + + @Test(groups="Integration") + public void testConcurrentBig() throws InterruptedException { + ConfigBag bag = ConfigBag.newInstance(); + bag.put(K1, "v1"); + bag.put(K2, "v2"); + bag.put(K3, "v3"); + runConcurrentTest(bag, 20, Duration.seconds(5)); + } + + private void runConcurrentTest(final ConfigBag bag, int numThreads, Duration time) throws InterruptedException { + List<Thread> threads = MutableList.of(); + final Map<Thread,Exception> exceptions = new ConcurrentHashMap<Thread,Exception>(); + final AtomicInteger successes = new AtomicInteger(); + for (int i=0; i<numThreads; i++) { + Thread t = new Thread() { + @Override + public void run() { + try { + while (!interrupted()) { + if (Math.random()<0.9) + bag.put(ConfigKeys.newStringConfigKey("k"+((int)(10*Math.random()))), "v"+((int)(10*Math.random()))); + if (Math.random()<0.8) + bag.get(ConfigKeys.newStringConfigKey("k"+((int)(10*Math.random())))); + if (Math.random()<0.2) + bag.copy(bag); + if (Math.random()<0.6) + bag.remove(ConfigKeys.newStringConfigKey("k"+((int)(10*Math.random())))); + successes.incrementAndGet(); + } + } catch (Exception e) { + exceptions.put(Thread.currentThread(), e); + Exceptions.propagateIfFatal(e); + } + } + }; + t.setName("ConfigBagTest-concurrent-thread-"+i); + threads.add(t); + } + for (Thread t: threads) t.start(); + time.countdownTimer().waitForExpiry(); + for (Thread t: threads) t.interrupt(); + for (Thread t: threads) t.join(); + Assert.assertTrue(exceptions.isEmpty(), "Got "+exceptions.size()+"/"+numThreads+" exceptions ("+successes.get()+" successful): "+exceptions); + } + +}
