GEODE-6829: Fixes synchronization in AttributesFactory (apache#3670)

Co-authored-by: Joris Melchior <[email protected]> Co-authored-by: Peter Tran <[email protected]>
b-xiang · Jun 10, 2019 · 7693689 · 7693689
1 parent d97f119
commit 7693689
Show file tree

Hide file tree

Showing 2 changed files with 65 additions and 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
@@ -1688,33 +1688,27 @@ public void setScope(Scope s) {
     @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,55 +1717,49 @@ public CacheListener<K, V> getCacheListener() {
     }
 
     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",
                     gatewaySenderId));
           }
           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",
                     asyncEventQueueId));
           }
           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
@@ -23,6 +23,7 @@
 
 import java.io.File;
 import java.util.Arrays;
+import java.util.Set;
 
 import org.junit.Test;
 
@@ -421,4 +422,36 @@ public void close() {}
   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);
+  }
 }