[geode] branch develop updated: GEODE-6587: fix race in useFirst (#3396)

[ds...@apache.org]

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

dschneider 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 d989fc9  GEODE-6587: fix race in useFirst (#3396)
d989fc9 is described below

commit d989fc939bf4d6d94e69338b9b77b74d12150ec6
Author: Darrel Schneider <ds...@pivotal.io>
AuthorDate: Tue Apr 2 17:07:51 2019 -0700

    GEODE-6587: fix race in useFirst (#3396)
    
    useFirst had a race condition in which the connection it
    returned might not match the predicate. This is because
    the predicate was only checked while the connection will
    still not active. Now a recheck is done after activating
    the connection. If, after activation it does not match, the
    connection is added back on the end of the deque.
---
 .../pooling/AvailableConnectionManager.java        |  9 +++++++-
 .../pooling/AvailableConnectionManagerTest.java    | 24 ++++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/AvailableConnectionManager.java b/geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/AvailableConnectionManager.java
index 6628266..5a32591 100644
--- a/geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/AvailableConnectionManager.java
+++ b/geode-core/src/main/java/org/apache/geode/cache/client/internal/pooling/AvailableConnectionManager.java
@@ -71,7 +71,14 @@ public class AvailableConnectionManager {
     while (connections.removeFirstOccurrence(equalsWithPredicate)) {
       PooledConnection connection = equalsWithPredicate.getConnectionThatMatched();
       if (connection.activate()) {
-        return connection;
+        // Need to recheck the predicate after we have activated.
+        // Until activated load conditioning can change the server
+        // we are connected to.
+        if (predicate.test(connection)) {
+          return connection;
+        } else {
+          addLast(connection, false);
+        }
       }
     }
     return null;
diff --git a/geode-core/src/test/java/org/apache/geode/cache/client/internal/pooling/AvailableConnectionManagerTest.java b/geode-core/src/test/java/org/apache/geode/cache/client/internal/pooling/AvailableConnectionManagerTest.java
index 1cc1c98..6ebdc4a 100644
--- a/geode-core/src/test/java/org/apache/geode/cache/client/internal/pooling/AvailableConnectionManagerTest.java
+++ b/geode-core/src/test/java/org/apache/geode/cache/client/internal/pooling/AvailableConnectionManagerTest.java
@@ -23,6 +23,8 @@ import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import java.util.concurrent.atomic.AtomicBoolean;
+
 import org.junit.Test;
 
 public class AvailableConnectionManagerTest {
@@ -117,6 +119,28 @@ public class AvailableConnectionManagerTest {
   }
 
   @Test
+  public void useFirstWithPredicateReturnsNullGivenManagerWithOneItemThatDoesNotMatchAfterBeingActivated() {
+    PooledConnection expected = createConnection();
+    when(expected.activate()).thenReturn(true);
+    instance.getDeque().addFirst(expected);
+    final AtomicBoolean firstTime = new AtomicBoolean(true);
+
+    PooledConnection result = instance.useFirst(c -> {
+      if (firstTime.get()) {
+        firstTime.set(false);
+        return true;
+      } else {
+        return false;
+      }
+    });
+
+    assertThat(result).isNull();
+    assertThat(instance.getDeque()).containsExactly(expected);
+    verify(expected).activate();
+    verify(expected).passivate(false);
+  }
+
+  @Test
   public void removeReturnsFalseGivenConnectionNotInManager() {
     instance.getDeque().clear();