[geode] branch support/1.15 updated: GEODE-10106: Use local ref to queueConnection. (#7740)

[nn...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

nnag pushed a commit to branch support/1.15
in repository https://gitbox.apache.org/repos/asf/geode.git


The following commit(s) were added to refs/heads/support/1.15 by this push:
     new 5f33d22d0c GEODE-10106: Use local ref to queueConnection. (#7740)
5f33d22d0c is described below

commit 5f33d22d0cbfb7e41ea585c74973c2f9ffb55c97
Author: Nabarun Nag <na...@users.noreply.github.com>
AuthorDate: Thu Jun 2 13:20:26 2022 -0700

    GEODE-10106: Use local ref to queueConnection. (#7740)
    
    * Using queueConnection local ref for multiple if checks
    * As it is a volatile variable, the value may become null mid checks.
    
    (cherry picked from commit b16dafa7128ec3a766f4edaa4d7e770113cddeb9)
---
 .../cache/client/internal/QueueManagerImpl.java    | 13 ++++++--
 .../client/internal/QueueManagerImplTest.java      | 37 ++++++++++++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueManagerImpl.java b/geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueManagerImpl.java
index 4474b6ba88..e16750f82b 100644
--- a/geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueManagerImpl.java
+++ b/geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueManagerImpl.java
@@ -862,8 +862,7 @@ public class QueueManagerImpl implements QueueManager {
       return;
     }
     final boolean isDebugEnabled = logger.isDebugEnabled();
-    if (queueConnections != null && queueConnections.getPrimary() != null
-        && !queueConnections.getPrimary().isDestroyed()) {
+    if (!isPrimaryRecoveryNeeded(queueConnections)) {
       if (isDebugEnabled) {
         logger.debug("Primary recovery not needed");
       }
@@ -966,6 +965,16 @@ public class QueueManagerImpl implements QueueManager {
     }
   }
 
+  static boolean isPrimaryRecoveryNeeded(final ConnectionList queueConnectionList) {
+    if (queueConnectionList != null) {
+      final Connection primaryConnection = queueConnectionList.getPrimary();
+      if (primaryConnection != null) {
+        return primaryConnection.isDestroyed();
+      }
+    }
+    return true;
+  }
+
   private QueueConnectionImpl initializeQueueConnection(Connection connection, boolean isPrimary,
       ClientUpdater failedUpdater) {
     QueueConnectionImpl queueConnection = null;
diff --git a/geode-core/src/test/java/org/apache/geode/cache/client/internal/QueueManagerImplTest.java b/geode-core/src/test/java/org/apache/geode/cache/client/internal/QueueManagerImplTest.java
index 264a834171..75cab96978 100644
--- a/geode-core/src/test/java/org/apache/geode/cache/client/internal/QueueManagerImplTest.java
+++ b/geode-core/src/test/java/org/apache/geode/cache/client/internal/QueueManagerImplTest.java
@@ -23,13 +23,19 @@ import static org.mockito.Mockito.inOrder;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import java.util.HashSet;
+import java.util.Set;
+
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.InOrder;
 
+import org.apache.geode.distributed.internal.ServerLocation;
+
 public class QueueManagerImplTest {
   private final InternalPool pool = mock(InternalPool.class, RETURNS_DEEP_STUBS);
   private final Endpoint endpoint = mock(Endpoint.class);
@@ -39,6 +45,9 @@ public class QueueManagerImplTest {
   private final ClientUpdater clientUpdater = mock(ClientUpdater.class);
   private QueueManagerImpl queueManager;
 
+  private final QueueManagerImpl.ConnectionList queueConnections = mock(
+      QueueManagerImpl.ConnectionList.class);
+
   @Before
   public void setup() {
     queueManager = new QueueManagerImpl(pool, null, null, null, 1, 1, null, null);
@@ -53,6 +62,34 @@ public class QueueManagerImplTest {
     when(backupEndpoint.isClosed()).thenReturn(false);
   }
 
+  @Test
+  public void whenPrimaryIsNotDestroyedThenPrimaryRecoveryIsNotNeeded() {
+    assertThat(queueManager.addToConnectionList((primary), true)).isTrue();
+    when(primary.isDestroyed()).thenReturn(false);
+    when(pool.getPoolOrCacheCancelInProgress()).thenReturn(null);
+    Set<ServerLocation> excludedServers = new HashSet<>();
+    queueManager.recoverPrimary(excludedServers);
+    verify(pool, times(2)).getPoolOrCacheCancelInProgress();
+  }
+
+  @Test
+  public void whenPrimaryIsNullThenPrimaryRecoveryIsNeeded() {
+    assertThat(QueueManagerImpl.isPrimaryRecoveryNeeded(null)).isTrue();
+  }
+
+  @Test
+  public void whenConnectionListIsNullThenPrimaryRecoveryIsNeeded() {
+    when(queueConnections.getPrimary()).thenReturn(null);
+    assertThat(QueueManagerImpl.isPrimaryRecoveryNeeded(queueConnections)).isTrue();
+  }
+
+  @Test
+  public void whenPrimaryIsDestroyedThenPrimaryRecoveryIsNeeded() {
+    when(queueConnections.getPrimary()).thenReturn(primary);
+    when(primary.isDestroyed()).thenReturn(true);
+    assertThat(QueueManagerImpl.isPrimaryRecoveryNeeded(queueConnections)).isTrue();
+  }
+
   @Test
   public void addNoClientUpdaterConnectionToConnectionListReturnsFalse() {
     when(primary.getUpdater()).thenReturn(null);