geode-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jinmeil...@apache.org
Subject [geode] branch develop updated: GEODE-6829: Fixes synchronization in AttributesFactory (#3670)
Date Mon, 10 Jun 2019 15:38:21 GMT
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.melchior@gmail.com>
AuthorDate: Mon Jun 10 11:38:08 2019 -0400

    GEODE-6829: Fixes synchronization in AttributesFactory (#3670)
    
    Co-authored-by: Joris Melchior <joris.melchior@gmail.com>
    Co-authored-by: Peter Tran <ptran@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);
+  }
 }


Mime
View raw message