You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by ahgittin <gi...@git.apache.org> on 2015/04/09 13:29:44 UTC
[GitHub] incubator-brooklyn pull request: make ConfigBag thread-safe
GitHub user ahgittin opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/589
make ConfigBag thread-safe
`ConfigBag` has not been thread-safe, but we have been accessing it from multiple threads. This can cause `ConcurrentModificationException` to be thrown in many places.
Such a higher-level shared class should be thread-safe, and this makes it so by synchronizing on the `ConfigBag` class appropriately. As we do not have iterators here it was pretty straightforward. (You get a copy of the map to iterate -- that has always been the case.)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ahgittin/incubator-brooklyn config-bag-thread-safe
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/589.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #589
----
commit 5e34f950f1d6a7875224112b60ae2de810b18feb
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2015-04-09T11:26:18Z
make ConfigBag thread-safe
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: make ConfigBag thread-safe
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/589#discussion_r28210046
--- Diff: core/src/main/java/brooklyn/util/config/ConfigBag.java ---
@@ -132,12 +166,12 @@ public String getDescription() {
/** 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() {
--- End diff --
+1 to phasing out the mutable accesses
as for `this`, it's a common and well understood pattern, and the sync dangers are no different to elsewhere. there may be times someone wants to batch items so it is handy to expose the object for synching. in time perhaps we'll replace with a RW lock, which would be the elegant thing, but since most `ConcurrentMap` impls don't do that it seems like overkill for now.
have added a comment in javadoc about synching and possibility of that changing
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: make ConfigBag thread-safe
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/589
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: make ConfigBag thread-safe
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/589#discussion_r28204838
--- Diff: core/src/main/java/brooklyn/util/config/ConfigBag.java ---
@@ -132,12 +166,12 @@ public String getDescription() {
/** 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() {
--- End diff --
Ah, I see that `getAllConfigMutable` means that we may need to synchronize on this. I really don't like our uses of `getAllConfigMutable`. Having public methods like that is an anti-pattern in my opinion.
Longer term, we should deprecate that and also `getUnusedConfigMutable`. We could have a `getMutex()` method for what to synchronize on, with that marked `@Beta`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: make ConfigBag thread-safe
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/589#issuecomment-92085852
Finished reviewing; one big thing (whether to synchronize on `this`) and the rest minor.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: make ConfigBag thread-safe
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/589#discussion_r28204775
--- Diff: core/src/main/java/brooklyn/util/config/ConfigBag.java ---
@@ -132,12 +166,12 @@ public String getDescription() {
/** 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() {
--- End diff --
Preference to not synchronize on `this` - if other code synchronizes on the object then leads to a risk of deadlock. It leaks the implementation detail. Better to declare a `private final` field, and synchronise on that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: make ConfigBag thread-safe
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/589#issuecomment-91795327
Looks good to merge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: make ConfigBag thread-safe
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/589#discussion_r28204869
--- Diff: 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() {
--- End diff --
Tests should use `ExecutorService executor = Executors.newCachedThreadPool()`, and in a finally block (or in tearDown) should do `executor.shutdownNow`. There are very few situations that folk should do `new Thread` these days, in my opinion.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: make ConfigBag thread-safe
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/589#discussion_r28204758
--- Diff: core/src/main/java/brooklyn/util/config/ConfigBag.java ---
@@ -96,14 +102,42 @@ public static ConfigBag newInstanceCopying(final ConfigBag 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;
--- End diff --
Was a bit surprised this didn't do `copy(parentBag)`. Should we leave that up to the caller, or do it inside `ConfigBagExtendingParent`'s constructor? I lean towards the latter because otherwise it hasn't really extended it. But no strong feelings.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: make ConfigBag thread-safe
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/589#discussion_r28209984
--- Diff: core/src/main/java/brooklyn/util/config/ConfigBag.java ---
@@ -96,14 +102,42 @@ public static ConfigBag newInstanceCopying(final ConfigBag 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;
--- End diff --
and do in constructor
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: make ConfigBag thread-safe
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/589#discussion_r28209947
--- Diff: core/src/main/java/brooklyn/util/config/ConfigBag.java ---
@@ -96,14 +102,42 @@ public static ConfigBag newInstanceCopying(final ConfigBag 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;
--- End diff --
let's make the class private
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---