[geode] branch develop updated: GEODE-9574: Sets cause to null on certain exceptions.

[jb...@apache.org]

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

jbarrett 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 ed00393  GEODE-9574: Sets cause to null on certain exceptions.
ed00393 is described below

commit ed003935689c157fdc52e9750bdbef21bd406569
Author: Jacob Barrett <jb...@pivotal.io>
AuthorDate: Wed Sep 1 16:44:10 2021 -0700

    GEODE-9574: Sets cause to null on certain exceptions.
    
    * Reverts some changes to exception handling in OpExecutorImpl that specifically depends on cause being null.
    * Coverts the handleException method to static for easier testing.
    * Adds test to assert these two exceptions get thrown without cause.
---
 .../cache/client/internal/OpExecutorImpl.java      | 29 +++++++---
 .../cache/client/internal/OpExecutorImplTest.java  | 64 ++++++++++++++++++++++
 2 files changed, 84 insertions(+), 9 deletions(-)

diff --git a/geode-core/src/main/java/org/apache/geode/cache/client/internal/OpExecutorImpl.java b/geode-core/src/main/java/org/apache/geode/cache/client/internal/OpExecutorImpl.java
index c5fe32a..6d75ece 100644
--- a/geode-core/src/main/java/org/apache/geode/cache/client/internal/OpExecutorImpl.java
+++ b/geode-core/src/main/java/org/apache/geode/cache/client/internal/OpExecutorImpl.java
@@ -497,10 +497,10 @@ public class OpExecutorImpl implements ExecutablePool {
    */
   protected void handleException(Throwable e, Connection conn, int retryCount,
       boolean finalAttempt) {
-    handleException(e, conn, retryCount, finalAttempt, false);
+    handleException(e, conn, retryCount, finalAttempt, false, cancelCriterion, endpointManager);
   }
 
-  protected void handleException(Op op, Throwable e, Connection conn, int retryCount,
+  private void handleException(Op op, Throwable e, Connection conn, int retryCount,
       boolean finalAttempt, boolean timeoutFatal) throws CacheRuntimeException {
     if (op instanceof AuthenticateUserOp.AuthenticateUserOpImpl) {
       if (e instanceof GemFireSecurityException) {
@@ -509,12 +509,15 @@ public class OpExecutorImpl implements ExecutablePool {
         throw (ServerRefusedConnectionException) e;
       }
     }
-    handleException(e, conn, retryCount, finalAttempt, timeoutFatal);
+    handleException(e, conn, retryCount, finalAttempt, timeoutFatal, cancelCriterion,
+        endpointManager);
   }
 
-  protected void handleException(final Throwable throwable, final Connection connection,
+  static void handleException(final @NotNull Throwable throwable,
+      final @NotNull Connection connection,
       final int retryCount, final boolean finalAttempt,
-      final boolean timeoutFatal) throws CacheRuntimeException {
+      final boolean timeoutFatal, final @NotNull CancelCriterion cancelCriterion,
+      final @NotNull EndpointManager endpointManager) throws CacheRuntimeException {
 
     cancelCriterion.checkCancelInProgress(throwable);
 
@@ -536,6 +539,8 @@ public class OpExecutorImpl implements ExecutablePool {
     boolean invalidateServer = true;
     boolean warn = true;
     boolean forceThrow = false;
+    Throwable cause = throwable;
+
     if (throwable instanceof MessageTooLargeException) {
       title = null;
       exceptionToThrow = new GemFireIOException("message is too large to transmit", throwable);
@@ -581,9 +586,13 @@ public class OpExecutorImpl implements ExecutablePool {
     } else if (throwable instanceof SocketTimeoutException) {
       invalidateServer = timeoutFatal;
       title = "socket timed out on client";
+      // specific cause checks will fail if cause is not null here
+      cause = null;
     } else if (throwable instanceof ConnectionDestroyedException) {
       invalidateServer = false;
       title = "connection was asynchronously destroyed";
+      // specific cause checks will fail if cause is not null here
+      cause = null;
     } else if (throwable instanceof java.io.EOFException) {
       title = "closed socket on server";
     } else if (throwable instanceof IOException) {
@@ -611,7 +620,8 @@ public class OpExecutorImpl implements ExecutablePool {
           || (t instanceof SerializationException) || (t instanceof CopyException)
           || (t instanceof GemFireSecurityException) || (t instanceof ServerOperationException)
           || (t instanceof TransactionException) || (t instanceof CancelException)) {
-        handleException(t, connection, retryCount, finalAttempt, timeoutFatal);
+        handleException(t, connection, retryCount, finalAttempt, timeoutFatal, cancelCriterion,
+            endpointManager);
         return;
       } else if (throwable instanceof ServerOperationException) {
         title = null; // no message
@@ -620,7 +630,8 @@ public class OpExecutorImpl implements ExecutablePool {
       } else if (throwable instanceof FunctionException) {
         if (t instanceof InternalFunctionInvocationTargetException) {
           // Client server to re-execute for node failure
-          handleException(t, connection, retryCount, finalAttempt, timeoutFatal);
+          handleException(t, connection, retryCount, finalAttempt, timeoutFatal, cancelCriterion,
+              endpointManager);
           return;
         } else {
           title = null; // no message
@@ -655,7 +666,7 @@ public class OpExecutorImpl implements ExecutablePool {
           }
         }
         if (forceThrow || finalAttempt) {
-          exceptionToThrow = new ServerConnectivityException(msg, throwable);
+          exceptionToThrow = new ServerConnectivityException(msg, cause);
         }
       }
     }
@@ -664,7 +675,7 @@ public class OpExecutorImpl implements ExecutablePool {
     }
   }
 
-  private StringBuilder getExceptionMessage(final String exceptionName, final int retryCount,
+  private static StringBuilder getExceptionMessage(final String exceptionName, final int retryCount,
       final boolean finalAttempt, final Connection connection) {
     final StringBuilder message = new StringBuilder(200);
     message.append("Pool unexpected ").append(exceptionName);
diff --git a/geode-core/src/test/java/org/apache/geode/cache/client/internal/OpExecutorImplTest.java b/geode-core/src/test/java/org/apache/geode/cache/client/internal/OpExecutorImplTest.java
new file mode 100644
index 0000000..fd737b6
--- /dev/null
+++ b/geode-core/src/test/java/org/apache/geode/cache/client/internal/OpExecutorImplTest.java
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.cache.client.internal;
+
+import static org.assertj.core.api.Assertions.assertThatNoException;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.Mockito.mock;
+
+import java.net.SocketTimeoutException;
+
+import org.junit.Test;
+
+import org.apache.geode.CancelCriterion;
+import org.apache.geode.cache.client.ServerConnectivityException;
+import org.apache.geode.cache.client.internal.pooling.ConnectionDestroyedException;
+
+public class OpExecutorImplTest {
+
+  @Test
+  public void handleExceptionWithSocketTimeoutExceptionDoesNotThrowException() {
+    assertThatNoException()
+        .isThrownBy(() -> OpExecutorImpl.handleException(new SocketTimeoutException(),
+            mock(Connection.class), 0, false,
+            false, mock(CancelCriterion.class), mock(EndpointManager.class)));
+  }
+
+  @Test
+  public void handleExceptionWithSocketTimeoutExceptionAndFinalAttemptThrowsServerConnectivityExceptionWithoutCause() {
+    assertThatThrownBy(() -> OpExecutorImpl.handleException(new SocketTimeoutException(),
+        mock(Connection.class), 0, true,
+        false, mock(CancelCriterion.class), mock(EndpointManager.class)))
+            .isInstanceOf(ServerConnectivityException.class).hasNoCause();
+  }
+
+  @Test
+  public void handleExceptionWithConnectionDestroyedExceptionDoesNotThrowException() {
+    assertThatNoException()
+        .isThrownBy(() -> OpExecutorImpl.handleException(new ConnectionDestroyedException(),
+            mock(Connection.class), 0, false,
+            false, mock(CancelCriterion.class), mock(EndpointManager.class)));
+  }
+
+  @Test
+  public void handleExceptionWithConnectionDestroyedExceptionAndFinalAttemptThrowsServerConnectivityExceptionWithoutCause() {
+    assertThatThrownBy(() -> OpExecutorImpl.handleException(new ConnectionDestroyedException(),
+        mock(Connection.class), 0, true,
+        false, mock(CancelCriterion.class), mock(EndpointManager.class)))
+            .isInstanceOf(ServerConnectivityException.class).hasNoCause();
+  }
+
+}