diff --git a/binder/src/main/java/io/grpc/binder/internal/PingTracker.java b/binder/src/main/java/io/grpc/binder/internal/PingTracker.java index ab20af4d6e..5a4300443b 100644 --- a/binder/src/main/java/io/grpc/binder/internal/PingTracker.java +++ b/binder/src/main/java/io/grpc/binder/internal/PingTracker.java @@ -99,7 +99,7 @@ final class PingTracker { private synchronized void fail(Status status) { if (!done) { done = true; - executor.execute(() -> callback.onFailure(status.asException())); + executor.execute(() -> callback.onFailure(status)); } } diff --git a/binder/src/test/java/io/grpc/binder/internal/PingTrackerTest.java b/binder/src/test/java/io/grpc/binder/internal/PingTrackerTest.java index 60e7c16310..c662cafe5f 100644 --- a/binder/src/test/java/io/grpc/binder/internal/PingTrackerTest.java +++ b/binder/src/test/java/io/grpc/binder/internal/PingTrackerTest.java @@ -96,7 +96,7 @@ public final class PingTrackerTest { private int numCallbacks; private boolean success; private boolean failure; - private Throwable failureException; + private Status failureStatus; private long roundtripTimeNanos; @Override @@ -107,10 +107,10 @@ public final class PingTrackerTest { } @Override - public synchronized void onFailure(Throwable failureException) { + public synchronized void onFailure(Status failureStatus) { numCallbacks += 1; failure = true; - this.failureException = failureException; + this.failureStatus = failureStatus; } public void assertNotCalled() { @@ -130,13 +130,13 @@ public final class PingTrackerTest { public void assertFailure(Status status) { assertThat(numCallbacks).isEqualTo(1); assertThat(failure).isTrue(); - assertThat(((StatusException) failureException).getStatus()).isSameInstanceAs(status); + assertThat(failureStatus).isSameInstanceAs(status); } public void assertFailure(Status.Code statusCode) { assertThat(numCallbacks).isEqualTo(1); assertThat(failure).isTrue(); - assertThat(((StatusException) failureException).getStatus().getCode()).isEqualTo(statusCode); + assertThat(failureStatus.getCode()).isEqualTo(statusCode); } } } diff --git a/core/src/main/java/io/grpc/internal/ClientTransport.java b/core/src/main/java/io/grpc/internal/ClientTransport.java index 98041cc6e7..fd0f30b8bf 100644 --- a/core/src/main/java/io/grpc/internal/ClientTransport.java +++ b/core/src/main/java/io/grpc/internal/ClientTransport.java @@ -22,6 +22,7 @@ import io.grpc.InternalChannelz.SocketStats; import io.grpc.InternalInstrumented; import io.grpc.Metadata; import io.grpc.MethodDescriptor; +import io.grpc.Status; import java.util.concurrent.Executor; import javax.annotation.concurrent.ThreadSafe; @@ -90,6 +91,6 @@ public interface ClientTransport extends InternalInstrumented { * * @param cause the cause of the ping failure */ - void onFailure(Throwable cause); + void onFailure(Status cause); } } diff --git a/core/src/main/java/io/grpc/internal/FailingClientTransport.java b/core/src/main/java/io/grpc/internal/FailingClientTransport.java index 5b31e6e507..37194c46a2 100644 --- a/core/src/main/java/io/grpc/internal/FailingClientTransport.java +++ b/core/src/main/java/io/grpc/internal/FailingClientTransport.java @@ -55,7 +55,7 @@ class FailingClientTransport implements ClientTransport { public void ping(final PingCallback callback, Executor executor) { executor.execute(new Runnable() { @Override public void run() { - callback.onFailure(error.asException()); + callback.onFailure(error); } }); } diff --git a/core/src/main/java/io/grpc/internal/Http2Ping.java b/core/src/main/java/io/grpc/internal/Http2Ping.java index d96ac3ef21..e352029562 100644 --- a/core/src/main/java/io/grpc/internal/Http2Ping.java +++ b/core/src/main/java/io/grpc/internal/Http2Ping.java @@ -18,6 +18,7 @@ package io.grpc.internal; import com.google.common.base.Stopwatch; import com.google.errorprone.annotations.concurrent.GuardedBy; +import io.grpc.Status; import io.grpc.internal.ClientTransport.PingCallback; import java.util.LinkedHashMap; import java.util.Map; @@ -62,7 +63,7 @@ public class Http2Ping { /** * If non-null, indicates the ping failed. */ - @GuardedBy("this") private Throwable failureCause; + @GuardedBy("this") private Status failureCause; /** * The round-trip time for the ping, in nanoseconds. This value is only meaningful when @@ -144,7 +145,7 @@ public class Http2Ping { * * @param failureCause the cause of failure */ - public void failed(Throwable failureCause) { + public void failed(Status failureCause) { Map callbacks; synchronized (this) { if (completed) { @@ -167,7 +168,7 @@ public class Http2Ping { * @param executor the executor used to invoke the callback * @param cause the cause of failure */ - public static void notifyFailed(PingCallback callback, Executor executor, Throwable cause) { + public static void notifyFailed(PingCallback callback, Executor executor, Status cause) { doExecute(executor, asRunnable(callback, cause)); } @@ -203,7 +204,7 @@ public class Http2Ping { * failure. */ private static Runnable asRunnable(final ClientTransport.PingCallback callback, - final Throwable failureCause) { + final Status failureCause) { return new Runnable() { @Override public void run() { diff --git a/core/src/main/java/io/grpc/internal/KeepAliveManager.java b/core/src/main/java/io/grpc/internal/KeepAliveManager.java index aed590c305..d831a09608 100644 --- a/core/src/main/java/io/grpc/internal/KeepAliveManager.java +++ b/core/src/main/java/io/grpc/internal/KeepAliveManager.java @@ -275,7 +275,7 @@ public class KeepAliveManager { public void onSuccess(long roundTripTimeNanos) {} @Override - public void onFailure(Throwable cause) { + public void onFailure(Status cause) { transport.shutdownNow(Status.UNAVAILABLE.withDescription( "Keepalive failed. The connection is likely gone")); } diff --git a/core/src/test/java/io/grpc/internal/KeepAliveManagerTest.java b/core/src/test/java/io/grpc/internal/KeepAliveManagerTest.java index 411a9fbe9f..3cf7bfcedf 100644 --- a/core/src/test/java/io/grpc/internal/KeepAliveManagerTest.java +++ b/core/src/test/java/io/grpc/internal/KeepAliveManagerTest.java @@ -127,7 +127,7 @@ public final class KeepAliveManagerTest { verify(transport).ping(pingCallbackCaptor.capture(), isA(Executor.class)); ClientTransport.PingCallback pingCallback = pingCallbackCaptor.getValue(); - pingCallback.onFailure(new Throwable()); + pingCallback.onFailure(Status.UNAVAILABLE.withDescription("I must write descriptions")); ArgumentCaptor statusCaptor = ArgumentCaptor.forClass(Status.class); verify(transport).shutdownNow(statusCaptor.capture()); diff --git a/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java b/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java index aea7ff4903..4a518895db 100644 --- a/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java +++ b/core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java @@ -181,7 +181,7 @@ public abstract class AbstractTransportTest { protected ManagedClientTransport.Listener mockClientTransportListener = mock(ManagedClientTransport.Listener.class); protected MockServerListener serverListener = new MockServerListener(); - private ArgumentCaptor throwableCaptor = ArgumentCaptor.forClass(Throwable.class); + private ArgumentCaptor statusCaptor = ArgumentCaptor.forClass(Status.class); protected final TestClientStreamTracer clientStreamTracer1 = new TestHeaderClientStreamTracer(); private final TestClientStreamTracer clientStreamTracer2 = new TestHeaderClientStreamTracer(); protected final ClientStreamTracer[] tracers = new ClientStreamTracer[] { @@ -626,8 +626,8 @@ public abstract class AbstractTransportTest { // Transport doesn't support ping, so this neither passes nor fails. assumeTrue(false); } - verify(mockPingCallback, timeout(TIMEOUT_MS)).onFailure(throwableCaptor.capture()); - Status status = Status.fromThrowable(throwableCaptor.getValue()); + verify(mockPingCallback, timeout(TIMEOUT_MS)).onFailure(statusCaptor.capture()); + Status status = statusCaptor.getValue(); assertSame(shutdownReason, status); } diff --git a/inprocess/src/main/java/io/grpc/inprocess/InProcessTransport.java b/inprocess/src/main/java/io/grpc/inprocess/InProcessTransport.java index 39ebe6e0ab..e294b4eb63 100644 --- a/inprocess/src/main/java/io/grpc/inprocess/InProcessTransport.java +++ b/inprocess/src/main/java/io/grpc/inprocess/InProcessTransport.java @@ -246,7 +246,7 @@ final class InProcessTransport implements ServerTransport, ConnectionClientTrans executor.execute(new Runnable() { @Override public void run() { - callback.onFailure(shutdownStatus.asRuntimeException()); + callback.onFailure(shutdownStatus); } }); } else { diff --git a/netty/src/main/java/io/grpc/netty/ClientTransportLifecycleManager.java b/netty/src/main/java/io/grpc/netty/ClientTransportLifecycleManager.java index 34f72ab97b..b4e53d5568 100644 --- a/netty/src/main/java/io/grpc/netty/ClientTransportLifecycleManager.java +++ b/netty/src/main/java/io/grpc/netty/ClientTransportLifecycleManager.java @@ -30,7 +30,6 @@ final class ClientTransportLifecycleManager { /** null iff !transportShutdown. */ private Status shutdownStatus; /** null iff !transportShutdown. */ - private Throwable shutdownThrowable; private boolean transportTerminated; public ClientTransportLifecycleManager(ManagedClientTransport.Listener listener) { @@ -72,7 +71,6 @@ final class ClientTransportLifecycleManager { return false; } shutdownStatus = s; - shutdownThrowable = s.asException(); return true; } @@ -97,7 +95,4 @@ final class ClientTransportLifecycleManager { return shutdownStatus; } - public Throwable getShutdownThrowable() { - return shutdownThrowable; - } } diff --git a/netty/src/main/java/io/grpc/netty/NettyClientHandler.java b/netty/src/main/java/io/grpc/netty/NettyClientHandler.java index 19f1903c0b..a5fa0f8007 100644 --- a/netty/src/main/java/io/grpc/netty/NettyClientHandler.java +++ b/netty/src/main/java/io/grpc/netty/NettyClientHandler.java @@ -499,7 +499,7 @@ class NettyClientHandler extends AbstractNettyHandler { streamStatus = lifecycleManager.getShutdownStatus(); } try { - cancelPing(lifecycleManager.getShutdownThrowable()); + cancelPing(lifecycleManager.getShutdownStatus()); // Report status to the application layer for any open streams connection().forEachActiveStream(new Http2StreamVisitor() { @Override @@ -593,13 +593,14 @@ class NettyClientHandler extends AbstractNettyHandler { */ private void createStream(CreateStreamCommand command, ChannelPromise promise) throws Exception { - if (lifecycleManager.getShutdownThrowable() != null) { + if (lifecycleManager.getShutdownStatus() != null) { command.stream().setNonExistent(); // The connection is going away (it is really the GOAWAY case), // just terminate the stream now. command.stream().transportReportStatus( lifecycleManager.getShutdownStatus(), RpcProgress.MISCARRIED, true, new Metadata()); - promise.setFailure(lifecycleManager.getShutdownThrowable()); + promise.setFailure(InternalStatus.asRuntimeExceptionWithoutStacktrace( + lifecycleManager.getShutdownStatus(), null)); return; } @@ -852,19 +853,21 @@ class NettyClientHandler extends AbstractNettyHandler { public void operationComplete(ChannelFuture future) throws Exception { if (future.isSuccess()) { transportTracer.reportKeepAliveSent(); + return; + } + Throwable cause = future.cause(); + Status status = lifecycleManager.getShutdownStatus(); + if (cause instanceof ClosedChannelException) { + if (status == null) { + status = Status.UNKNOWN.withDescription("Ping failed but for unknown reason.") + .withCause(future.cause()); + } } else { - Throwable cause = future.cause(); - if (cause instanceof ClosedChannelException) { - cause = lifecycleManager.getShutdownThrowable(); - if (cause == null) { - cause = Status.UNKNOWN.withDescription("Ping failed but for unknown reason.") - .withCause(future.cause()).asException(); - } - } - finalPing.failed(cause); - if (ping == finalPing) { - ping = null; - } + status = Utils.statusFromThrowable(cause); + } + finalPing.failed(status); + if (ping == finalPing) { + ping = null; } } }); @@ -963,9 +966,9 @@ class NettyClientHandler extends AbstractNettyHandler { } } - private void cancelPing(Throwable t) { + private void cancelPing(Status s) { if (ping != null) { - ping.failed(t); + ping.failed(s); ping = null; } } diff --git a/netty/src/main/java/io/grpc/netty/NettyClientTransport.java b/netty/src/main/java/io/grpc/netty/NettyClientTransport.java index 86d8991ba9..e03989e990 100644 --- a/netty/src/main/java/io/grpc/netty/NettyClientTransport.java +++ b/netty/src/main/java/io/grpc/netty/NettyClientTransport.java @@ -165,7 +165,7 @@ class NettyClientTransport implements ConnectionClientTransport { executor.execute(new Runnable() { @Override public void run() { - callback.onFailure(statusExplainingWhyTheChannelIsNull.asException()); + callback.onFailure(statusExplainingWhyTheChannelIsNull); } }); return; @@ -177,7 +177,7 @@ class NettyClientTransport implements ConnectionClientTransport { public void operationComplete(ChannelFuture future) throws Exception { if (!future.isSuccess()) { Status s = statusFromFailedFuture(future); - Http2Ping.notifyFailed(callback, executor, s.asException()); + Http2Ping.notifyFailed(callback, executor, s); } } }; diff --git a/netty/src/test/java/io/grpc/netty/NettyClientHandlerTest.java b/netty/src/test/java/io/grpc/netty/NettyClientHandlerTest.java index 945c6c1267..f8fbeea9b8 100644 --- a/netty/src/test/java/io/grpc/netty/NettyClientHandlerTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyClientHandlerTest.java @@ -59,7 +59,6 @@ import io.grpc.Attributes; import io.grpc.CallOptions; import io.grpc.Metadata; import io.grpc.Status; -import io.grpc.StatusException; import io.grpc.internal.AbstractStream; import io.grpc.internal.ClientStreamListener; import io.grpc.internal.ClientStreamListener.RpcProgress; @@ -812,9 +811,7 @@ public class NettyClientHandlerTest extends NettyHandlerTestBase