Blocking clients wrap checked failures with DialogueException (#759)

Blocking clients wrap checked failures with UnknownRemoteException

changelog/@unreleased/pr-759.v2.yml

@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: Blocking clients wrap checked failures with a custom internal `DialogueException`.
+  links:
+  - https://github.com/palantir/dialogue/pull/759

dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/DefaultClients.java

@@ -17,12 +17,10 @@
 package com.palantir.conjure.java.dialogue.serde;
 
 import com.google.common.net.HttpHeaders;
-import com.google.common.util.concurrent.ExecutionError;
 import com.google.common.util.concurrent.FutureCallback;
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.MoreExecutors;
-import com.google.common.util.concurrent.UncheckedExecutionException;
 import com.palantir.conjure.java.api.errors.RemoteException;
 import com.palantir.conjure.java.api.errors.UnknownRemoteException;
 import com.palantir.dialogue.Channel;
@@ -96,7 +94,6 @@ private static ListenableFuture<Response> closeRequestBodyOnCompletion(
     }
 
     @Override
-    @SuppressWarnings("ThrowError") // match behavior of Futures.getUnchecked(Future)
     public <T> T block(ListenableFuture<T> future) {
         try {
             return future.get();
@@ -108,7 +105,6 @@ public <T> T block(ListenableFuture<T> future) {
             throw new SafeRuntimeException("Interrupted waiting for future", e);
         } catch (ExecutionException e) {
             Throwable cause = e.getCause();
-            String message = cause.getMessage();
 
             // TODO(jellis): can consider propagating other relevant exceptions (eg: HttpConnectTimeoutException)
             // see HttpClientImpl#send(HttpRequest req, BodyHandler<T> responseHandler)
@@ -129,9 +125,10 @@ public <T> T block(ListenableFuture<T> future) {
 
             // This matches the behavior in Futures.getUnchecked(Future)
             if (cause instanceof Error) {
-                throw new ExecutionError(cause.getMessage(), (Error) cause);
+                cause.addSuppressed(new SafeRuntimeException("Rethrown by dialogue"));
+                throw (Error) cause;
             }
-            throw new UncheckedExecutionException(message, cause);
+            throw new DialogueException(cause);
         }
     }
 

dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/DialogueException.java

@@ -0,0 +1,41 @@
+/*
+ * (c) Copyright 2020 Palantir Technologies 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.palantir.conjure.java.dialogue.serde;
+
+import com.google.common.collect.ImmutableList;
+import com.palantir.logsafe.Arg;
+import com.palantir.logsafe.SafeLoggable;
+import java.util.List;
+
+/** Internal marker type for failure legibility, this type is not meant to be handled directly. */
+final class DialogueException extends RuntimeException implements SafeLoggable {
+    private static final String MESSAGE = "Dialogue transport failure";
+
+    DialogueException(Throwable cause) {
+        super(MESSAGE, cause);
+    }
+
+    @Override
+    public String getLogMessage() {
+        return MESSAGE;
+    }
+
+    @Override
+    public List<Arg<?>> getArgs() {
+        return ImmutableList.of();
+    }
+}

dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsBlockingTest.java

@@ -21,11 +21,9 @@
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 
-import com.google.common.util.concurrent.ExecutionError;
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.SettableFuture;
-import com.google.common.util.concurrent.UncheckedExecutionException;
 import com.palantir.conjure.java.api.errors.ErrorType;
 import com.palantir.conjure.java.api.errors.RemoteException;
 import com.palantir.conjure.java.api.errors.SerializableError;
@@ -89,18 +87,16 @@ public void testException() {
         ListenableFuture<Object> failedFuture = Futures.immediateFailedFuture(exception);
 
         assertThatThrownBy(() -> DefaultClients.INSTANCE.block(failedFuture))
-                .isInstanceOf(UncheckedExecutionException.class)
-                .hasCauseInstanceOf(Exception.class);
+                .satisfies(value -> value.getClass().getSimpleName().contains("DialogueException"))
+                .hasCause(exception);
     }
 
     @Test
     public void testError() {
         Error error = new Error();
         ListenableFuture<Object> failedFuture = Futures.immediateFailedFuture(error);
 
-        assertThatThrownBy(() -> DefaultClients.INSTANCE.block(failedFuture))
-                .isInstanceOf(ExecutionError.class)
-                .hasCauseInstanceOf(Error.class);
+        assertThatThrownBy(() -> DefaultClients.INSTANCE.block(failedFuture)).isSameAs(error);
     }
 
     @Test

dialogue-test-common/src/main/java/com/palantir/dialogue/AbstractSampleServiceClientTest.java

@@ -195,7 +195,8 @@ public void testBlocking_throwsOnConnectError() throws Exception {
         server.shutdown();
         assertThatThrownBy(() -> blockingClient.objectToObject(HEADER, PATH, QUERY, BODY))
                 .isInstanceOf(RuntimeException.class)
-                .hasCauseInstanceOf(ConnectException.class)
+                .getCause()
+                .isInstanceOf(ConnectException.class)
                 .hasMessageMatching(".*((Connection refused)|(Failed to connect)).*");
     }