This is an automated email from the ASF dual-hosted git repository. jinmeiliao pushed a commit to branch develop in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/develop by this push: new 7693689 GEODE-6829: Fixes synchronization in AttributesFactory (#3670) 7693689 is described below commit 769368999c24ec469b6ca8d323cd8de960b069fb Author: Joris Melchior <joris.melch...@gmail.com> AuthorDate: Mon Jun 10 11:38:08 2019 -0400 GEODE-6829: Fixes synchronization in AttributesFactory (#3670) Co-authored-by: Joris Melchior <joris.melch...@gmail.com> Co-authored-by: Peter Tran <pt...@gmail.com> --- .../org/apache/geode/cache/AttributesFactory.java | 76 +++++++++------------- .../geode/cache/AttributesFactoryJUnitTest.java | 33 ++++++++++ 2 files changed, 65 insertions(+), 44 deletions(-) diff --git a/geode-core/src/main/java/org/apache/geode/cache/AttributesFactory.java b/geode-core/src/main/java/org/apache/geode/cache/AttributesFactory.java index c000227..c9ff9e2 100644 --- a/geode-core/src/main/java/org/apache/geode/cache/AttributesFactory.java +++ b/geode-core/src/main/java/org/apache/geode/cache/AttributesFactory.java @@ -1688,33 +1688,27 @@ public class AttributesFactory<K, V> { @Override @SuppressWarnings("unchecked") public CacheListener<K, V>[] getCacheListeners() { - ArrayList<CacheListener<K, V>> listeners = this.cacheListeners; - if (listeners == null) { - return (CacheListener<K, V>[]) EMPTY_LISTENERS; - } else { - synchronized (listeners) { - if (listeners.size() == 0) { - return (CacheListener<K, V>[]) EMPTY_LISTENERS; - } else { - CacheListener<K, V>[] result = new CacheListener[listeners.size()]; - listeners.toArray(result); - return result; - } + synchronized (this) { + if (this.cacheListeners == null || this.cacheListeners.isEmpty()) { + return (CacheListener<K, V>[]) EMPTY_LISTENERS; + } else { + CacheListener<K, V>[] result = new CacheListener[this.cacheListeners.size()]; + this.cacheListeners.toArray(result); + return result; } } } @Override public CacheListener<K, V> getCacheListener() { - ArrayList<CacheListener<K, V>> listeners = this.cacheListeners; - if (listeners == null) { - return null; - } - synchronized (listeners) { - if (listeners.size() == 0) { + synchronized (this) { + if (this.cacheListeners == null) { return null; } - if (listeners.size() == 1) { + if (this.cacheListeners.size() == 0) { + return null; + } + if (this.cacheListeners.size() == 1) { return this.cacheListeners.get(0); } } @@ -1723,27 +1717,23 @@ public class AttributesFactory<K, V> { } protected void addCacheListener(CacheListener<K, V> aListener) { - ArrayList<CacheListener<K, V>> listeners = this.cacheListeners; - if (listeners == null) { - ArrayList<CacheListener<K, V>> al = new ArrayList<CacheListener<K, V>>(1); - al.add(aListener); - this.cacheListeners = al; - } else { - synchronized (listeners) { - listeners.add(aListener); + synchronized (this) { + if (this.cacheListeners == null) { + this.cacheListeners = new ArrayList<CacheListener<K, V>>(1); + this.cacheListeners.add(aListener); + } else { + this.cacheListeners.add(aListener); } + setHasCacheListeners(true); } - setHasCacheListeners(true); } public void addGatewaySenderId(String gatewaySenderId) { - if (this.gatewaySenderIds == null) { - this.gatewaySenderIds = new CopyOnWriteArraySet<String>(); - this.gatewaySenderIds.add(gatewaySenderId); - } else { - synchronized (this.gatewaySenderIds) { // TODO: revisit this - // synchronization : added as per - // above code + synchronized (this) { + if (this.gatewaySenderIds == null) { + this.gatewaySenderIds = new CopyOnWriteArraySet<String>(); + this.gatewaySenderIds.add(gatewaySenderId); + } else { if (this.gatewaySenderIds.contains(gatewaySenderId)) { throw new IllegalArgumentException( String.format("gateway-sender-id %s is already added", @@ -1751,18 +1741,16 @@ public class AttributesFactory<K, V> { } this.gatewaySenderIds.add(gatewaySenderId); } + setHasGatewaySenderIds(true); } - setHasGatewaySenderIds(true); } public void addAsyncEventQueueId(String asyncEventQueueId) { - if (this.asyncEventQueueIds == null) { - this.asyncEventQueueIds = new CopyOnWriteArraySet<String>(); - this.asyncEventQueueIds.add(asyncEventQueueId); - } else { - synchronized (this.asyncEventQueueIds) { // TODO: revisit this - // synchronization : added as per - // above code + synchronized (this) { + if (this.asyncEventQueueIds == null) { + this.asyncEventQueueIds = new CopyOnWriteArraySet<String>(); + this.asyncEventQueueIds.add(asyncEventQueueId); + } else { if (this.asyncEventQueueIds.contains(asyncEventQueueId)) { throw new IllegalArgumentException( String.format("async-event-queue-id %s is already added", @@ -1770,8 +1758,8 @@ public class AttributesFactory<K, V> { } this.asyncEventQueueIds.add(asyncEventQueueId); } + setHasAsyncEventListeners(true); } - setHasAsyncEventListeners(true); } @Override diff --git a/geode-core/src/test/java/org/apache/geode/cache/AttributesFactoryJUnitTest.java b/geode-core/src/test/java/org/apache/geode/cache/AttributesFactoryJUnitTest.java index e6d0270..95d91a8 100644 --- a/geode-core/src/test/java/org/apache/geode/cache/AttributesFactoryJUnitTest.java +++ b/geode-core/src/test/java/org/apache/geode/cache/AttributesFactoryJUnitTest.java @@ -23,6 +23,7 @@ import static org.junit.Assert.fail; import java.io.File; import java.util.Arrays; +import java.util.Set; import org.junit.Test; @@ -421,4 +422,36 @@ public class AttributesFactoryJUnitTest { private static class MyCacheListener extends CacheListenerAdapter { // empty impl } + + @Test + public void addsGatewaySenderId() { + AttributesFactory factory = new AttributesFactory(); + factory.addGatewaySenderId("someSenderId"); + RegionAttributes regionAttributes = factory.create(); + Set gatewaySenderIds = regionAttributes.getGatewaySenderIds(); + + assertTrue(gatewaySenderIds.contains("someSenderId")); + } + + @Test(expected = IllegalArgumentException.class) + public void addingNullGatewaySenderIdThrowsException() { + AttributesFactory factory = new AttributesFactory(); + factory.addGatewaySenderId(null); + } + + @Test + public void addsAsyncEventQueueId() { + AttributesFactory factory = new AttributesFactory(); + factory.addAsyncEventQueueId("someId"); + RegionAttributes regionAttributes = factory.create(); + Set asyncEventQueueIds = regionAttributes.getAsyncEventQueueIds(); + + assertTrue(asyncEventQueueIds.contains("someId")); + } + + @Test(expected = IllegalArgumentException.class) + public void addingNullAsyncEventQueueIdThrowsException() { + AttributesFactory factory = new AttributesFactory(); + factory.addAsyncEventQueueId(null); + } }