Fix exception wrapping for member/client comm (#17203)

* Fix exception wrapping for member/client comm Exceptions that comes from remote are wrapped to HazelcastEx if they are not RuntimeExceptions. That prevents clients to act accordingly to exceptions. With this pr, we are removing the wrapping. fixes #17022 * Changed RuntimeExceptionFactory to ExceptionWrapper * simplify test
hazelcast · Jul 14, 2020 · 67f9456 · 67f9456
1 parent 2767be4
commit 67f9456
Show file tree

Hide file tree

Showing 5 changed files with 121 additions and 38 deletions.
diff --git a/hazelcast-client/src/main/java/com/hazelcast/client/proxy/txn/ClientTransactionUtil.java b/hazelcast-client/src/main/java/com/hazelcast/client/proxy/txn/ClientTransactionUtil.java
@@ -21,7 +21,7 @@
 import com.hazelcast.client.spi.impl.ClientInvocation;
 import com.hazelcast.nio.Connection;
 import com.hazelcast.transaction.TransactionException;
-import com.hazelcast.util.ExceptionUtil.RuntimeExceptionFactory;
+import com.hazelcast.util.ExceptionUtil.ExceptionWrapper;
 
 import java.util.concurrent.Future;
 
@@ -32,8 +32,8 @@
  */
 public final class ClientTransactionUtil {
 
-    private static final RuntimeExceptionFactory TRANSACTION_EXCEPTION_FACTORY =
-            new RuntimeExceptionFactory() {
+    private static final ExceptionWrapper<RuntimeException> TRANSACTION_EXCEPTION_WRAPPER =
+            new ExceptionWrapper<RuntimeException>() {
                 @Override
                 public RuntimeException create(Throwable throwable, String message) {
                     return new TransactionException(message, throwable);
@@ -56,7 +56,7 @@ public static ClientMessage invoke(ClientMessage request, String objectName, Haz
             final Future<ClientMessage> future = clientInvocation.invoke();
             return future.get();
         } catch (Exception e) {
-            throw rethrow(e, TRANSACTION_EXCEPTION_FACTORY);
+            throw rethrow(e, TRANSACTION_EXCEPTION_WRAPPER);
         }
     }
 }
diff --git a/...lient/src/test/java/com/hazelcast/client/executor/ClientExecutorServiceExceptionTest.java b/...lient/src/test/java/com/hazelcast/client/executor/ClientExecutorServiceExceptionTest.java
@@ -0,0 +1,71 @@
+/*
+ * Copyright (c) 2008-2020, Hazelcast, Inc. All Rights Reserved.
+ *
+ * Licensed 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 com.hazelcast.client.executor;
+
+import com.hazelcast.client.config.ClientConfig;
+import com.hazelcast.client.test.TestHazelcastFactory;
+import com.hazelcast.core.HazelcastInstance;
+import com.hazelcast.core.IExecutorService;
+import com.hazelcast.spi.exception.RetryableIOException;
+import com.hazelcast.test.HazelcastParallelClassRunner;
+import com.hazelcast.test.annotation.ParallelTest;
+import com.hazelcast.test.annotation.QuickTest;
+import org.junit.After;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
+
+import java.io.Serializable;
+import java.util.concurrent.Callable;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import static org.junit.Assert.assertEquals;
+
+@RunWith(HazelcastParallelClassRunner.class)
+@Category({QuickTest.class, ParallelTest.class})
+public class ClientExecutorServiceExceptionTest {
+
+    private final TestHazelcastFactory hazelcastFactory = new TestHazelcastFactory();
+
+    @After
+    public void tearDown() {
+        hazelcastFactory.terminateAll();
+    }
+
+    public static class SecondTimeSuccessCallable implements Serializable, Callable {
+        private static AtomicInteger runCount = new AtomicInteger();
+
+        @Override
+        public Object call() throws Exception {
+            if (runCount.incrementAndGet() == 1) {
+                throw new RetryableIOException();
+            }
+            return "SUCCESS";
+        }
+    }
+
+    @Test
+    public void testRetriableIOException() throws Throwable {
+        hazelcastFactory.newHazelcastInstance();
+        ClientConfig clientConfig = new ClientConfig();
+        clientConfig.getNetworkConfig().setSmartRouting(false);
+        HazelcastInstance client = hazelcastFactory.newHazelcastClient(clientConfig);
+        IExecutorService executorService = client.getExecutorService("test");
+
+        assertEquals("SUCCESS", executorService.submit(new SecondTimeSuccessCallable()).get());
+    }
+}
diff --git a/hazelcast/src/main/java/com/hazelcast/client/impl/protocol/task/AbstractMessageTask.java b/hazelcast/src/main/java/com/hazelcast/client/impl/protocol/task/AbstractMessageTask.java
@@ -36,6 +36,7 @@
 import com.hazelcast.security.SecurityContext;
 import com.hazelcast.spi.exception.RetryableHazelcastException;
 import com.hazelcast.spi.impl.NodeEngineImpl;
+import com.hazelcast.util.ExceptionUtil;
 
 import java.lang.reflect.Field;
 import java.security.Permission;
@@ -52,6 +53,13 @@
  */
 public abstract class AbstractMessageTask<P> implements MessageTask, SecureRequest {
 
+    private static final ExceptionUtil.ExceptionWrapper<Throwable> NOOP_WRAPPER =
+            new ExceptionUtil.ExceptionWrapper<Throwable>() {
+                @Override
+                public Throwable create(Throwable throwable, String message) {
+                    return throwable;
+                }
+            };
     private static final List<Class<? extends Throwable>> NON_PEELABLE_EXCEPTIONS =
             Arrays.asList(Error.class, MemberLeftException.class);
 
@@ -218,7 +226,8 @@ protected void sendClientMessage(Object key, ClientMessage resultClientMessage)
 
     protected void sendClientMessage(Throwable throwable) {
         ClientExceptions exceptionFactory = clientEngine.getClientExceptions();
-        ClientMessage exception = exceptionFactory.createExceptionMessage(peelIfNeeded(throwable));
+        Throwable throwable1 = peelIfNeeded(throwable);
+        ClientMessage exception = exceptionFactory.createExceptionMessage(throwable1);
         sendClientMessage(exception);
     }
 
@@ -289,6 +298,6 @@ private Throwable peelIfNeeded(Throwable t) {
             }
         }
 
-        return peel(t);
+        return peel(t, null, null, NOOP_WRAPPER);
     }
 }
diff --git a/hazelcast/src/main/java/com/hazelcast/util/ExceptionUtil.java b/hazelcast/src/main/java/com/hazelcast/util/ExceptionUtil.java
@@ -31,22 +31,23 @@ public final class ExceptionUtil {
 
     private static final String EXCEPTION_SEPARATOR = "------ submitted from ------";
     private static final String EXCEPTION_MESSAGE_SEPARATOR = "------ %MSG% ------";
-    private static final RuntimeExceptionFactory HAZELCAST_EXCEPTION_FACTORY = new RuntimeExceptionFactory() {
-        @Override
-        public RuntimeException create(Throwable throwable, String message) {
-            if (message != null) {
-                return new HazelcastException(message, throwable);
-            } else {
-                return new HazelcastException(throwable);
-            }
-        }
-    };
+    private static final ExceptionWrapper<RuntimeException> HAZELCAST_EXCEPTION_WRAPPER =
+            new ExceptionWrapper<RuntimeException>() {
+                @Override
+                public RuntimeException create(Throwable throwable, String message) {
+                    if (message != null) {
+                        return new HazelcastException(message, throwable);
+                    } else {
+                        return new HazelcastException(throwable);
+                    }
+                }
+            };
 
     /**
      * Interface used by rethrow/peel to wrap the peeled exception
      */
-    public interface RuntimeExceptionFactory {
-        RuntimeException create(Throwable throwable, String message);
+    public interface ExceptionWrapper<T extends Throwable> {
+        T create(Throwable throwable, String message);
     }
 
     private ExceptionUtil() {
@@ -66,7 +67,7 @@ public static String toString(Throwable cause) {
     }
 
     public static RuntimeException peel(final Throwable t) {
-        return (RuntimeException) peel(t, null, null, HAZELCAST_EXCEPTION_FACTORY);
+        return (RuntimeException) peel(t, null, null, HAZELCAST_EXCEPTION_WRAPPER);
     }
 
     /**
@@ -84,54 +85,56 @@ public static RuntimeException peel(final Throwable t) {
      * @return the peeled {@code Throwable}
      */
     public static <T extends Throwable> Throwable peel(final Throwable t, Class<T> allowedType, String message) {
-        return peel(t, allowedType, message, HAZELCAST_EXCEPTION_FACTORY);
+        return peel(t, allowedType, message, HAZELCAST_EXCEPTION_WRAPPER);
     }
 
     /**
-     * Processes {@code Throwable t} so that the returned {@code Throwable}'s type matches {@code allowedType} or
-     * {@code RuntimeException}. Processing may include unwrapping {@code t}'s cause hierarchy, wrapping it in a
-     * {@code RuntimeException} created by using runtimeExceptionFactory or just returning the same instance {@code t}
+     * Processes {@code Throwable t} so that the returned {@code Throwable}'s type matches {@code allowedType},
+     * {@code RuntimeException} or any {@code Throwable} returned by `exceptionWrapper`
+     * Processing may include unwrapping {@code t}'s cause hierarchy, wrapping it in a exception
+     * created by using exceptionWrapper or just returning the same instance {@code t}
      * if it is already an instance of {@code RuntimeException}.
      *
-     * @param t                       {@code Throwable} to be peeled
-     * @param allowedType             the type expected to be returned; when {@code null}, this method returns instances
-     *                                of {@code RuntimeException}
-     * @param message                 if not {@code null}, used as the message in {@code RuntimeException} that
-     *                                may wrap the peeled {@code Throwable}
-     * @param runtimeExceptionFactory wraps the peeled code using this runtimeExceptionFactory
-     * @param <T>                     expected type of {@code Throwable}
+     * @param t                {@code Throwable} to be peeled
+     * @param allowedType      the type expected to be returned; when {@code null}, this method returns instances
+     *                         of {@code RuntimeException} or <W>
+     * @param message          if not {@code null}, used as the message in {@code RuntimeException} that
+     *                         may wrap the peeled {@code Throwable}
+     * @param exceptionWrapper wraps the peeled code using this exceptionWrapper
+     * @param <W>              Type of the wrapper exception in exceptionWrapper
+     * @param <T>              allowed type of {@code Throwable}
      * @return the peeled {@code Throwable}
      */
-    public static <T extends Throwable> Throwable peel(final Throwable t, Class<T> allowedType,
-                                                       String message, RuntimeExceptionFactory runtimeExceptionFactory) {
+    public static <T, W extends Throwable> Throwable peel(final Throwable t, Class<T> allowedType,
+                                                          String message, ExceptionWrapper<W> exceptionWrapper) {
         if (t instanceof RuntimeException) {
             return t;
         }
 
         if (t instanceof ExecutionException || t instanceof InvocationTargetException) {
             final Throwable cause = t.getCause();
             if (cause != null) {
-                return peel(cause, allowedType, message, runtimeExceptionFactory);
+                return peel(cause, allowedType, message, exceptionWrapper);
             } else {
-                return runtimeExceptionFactory.create(t, message);
+                return exceptionWrapper.create(t, message);
             }
         }
 
         if (allowedType != null && allowedType.isAssignableFrom(t.getClass())) {
             return t;
         }
 
-        return runtimeExceptionFactory.create(t, message);
+        return exceptionWrapper.create(t, message);
     }
 
     public static RuntimeException rethrow(final Throwable t) {
         rethrowIfError(t);
         throw peel(t);
     }
 
-    public static RuntimeException rethrow(final Throwable t, RuntimeExceptionFactory runtimeExceptionFactory) {
+    public static RuntimeException rethrow(final Throwable t, ExceptionWrapper<RuntimeException> exceptionWrapper) {
         rethrowIfError(t);
-        throw (RuntimeException) peel(t, null, null, runtimeExceptionFactory);
+        throw (RuntimeException) peel(t, null, null, exceptionWrapper);
     }
 
     public static <T extends Throwable> RuntimeException rethrow(final Throwable t, Class<T> allowedType) throws T {

diff --git a/hazelcast/src/test/java/com/hazelcast/util/ExceptionUtilTest.java b/hazelcast/src/test/java/com/hazelcast/util/ExceptionUtilTest.java
@@ -77,7 +77,7 @@ public void testPeel_whenThrowableIsExecutionExceptionWithNullCause_thenReturnHa
     public void testPeel_whenThrowableIsExecutionExceptionWithCustomFactory_thenReturnCustomException() {
         IOException expectedException = new IOException();
         RuntimeException result = (RuntimeException) ExceptionUtil.peel(new ExecutionException(expectedException),
-                null, null, new ExceptionUtil.RuntimeExceptionFactory() {
+                null, null, new ExceptionUtil.ExceptionWrapper() {
                     @Override
                     public RuntimeException create(Throwable throwable, String message) {
                         return new IllegalStateException(message, throwable);