From 6844e2f78292c9ba3d073986d9a79df0521569de Mon Sep 17 00:00:00 2001 From: buchgr Date: Mon, 12 Sep 2016 11:11:58 +0200 Subject: [PATCH 1/2] netty: Fix receipt of ClosedChannelException instead of actual error. Fixes #1330. Our API allows pings to be send even after the transport has been shutdown. We currently don't handle the case, where the Netty channel has been closed but the NettyClientHandler has not yet been removed from the pipeline, correctly. That is, we need to query the shutdown status whenever we receive a ClosedChannelException. Also, some cleanup. --- core/src/main/java/io/grpc/internal/Http2Ping.java | 2 +- .../main/java/io/grpc/netty/NettyClientHandler.java | 7 ++++++- .../java/io/grpc/netty/NettyClientTransport.java | 12 ++++++------ netty/src/main/java/io/grpc/netty/Utils.java | 5 +---- .../grpc/internal/testing/AbstractTransportTest.java | 9 +-------- 5 files changed, 15 insertions(+), 20 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/Http2Ping.java b/core/src/main/java/io/grpc/internal/Http2Ping.java index 2b4df888af9..550e7bd36af 100644 --- a/core/src/main/java/io/grpc/internal/Http2Ping.java +++ b/core/src/main/java/io/grpc/internal/Http2Ping.java @@ -174,7 +174,7 @@ public void failed(Throwable failureCause) { this.callbacks = null; } for (Map.Entry entry : callbacks.entrySet()) { - doExecute(entry.getValue(), asRunnable(entry.getKey(), failureCause)); + notifyFailed(entry.getKey(), entry.getValue(), failureCause); } } diff --git a/netty/src/main/java/io/grpc/netty/NettyClientHandler.java b/netty/src/main/java/io/grpc/netty/NettyClientHandler.java index 054d79e59a5..7b9ccffa9a7 100644 --- a/netty/src/main/java/io/grpc/netty/NettyClientHandler.java +++ b/netty/src/main/java/io/grpc/netty/NettyClientHandler.java @@ -79,6 +79,7 @@ import io.netty.handler.codec.http2.StreamBufferingEncoder; import io.netty.handler.logging.LogLevel; +import java.nio.channels.ClosedChannelException; import java.util.Random; import java.util.concurrent.Executor; import java.util.logging.Level; @@ -472,7 +473,11 @@ private void sendPingFrame(ChannelHandlerContext ctx, SendPingCommand msg, @Override public void operationComplete(ChannelFuture future) throws Exception { if (!future.isSuccess()) { - finalPing.failed(future.cause()); + Throwable cause = future.cause(); + if (cause instanceof ClosedChannelException) { + cause = lifecycleManager.getShutdownThrowable(); + } + finalPing.failed(cause); if (ping == finalPing) { 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 76fffc80e01..bd036ffb2c7 100644 --- a/netty/src/main/java/io/grpc/netty/NettyClientTransport.java +++ b/netty/src/main/java/io/grpc/netty/NettyClientTransport.java @@ -99,6 +99,8 @@ class NettyClientTransport implements ConnectionClientTransport { @Override public void ping(final PingCallback callback, final Executor executor) { + // The promise and listener always succeed in NettyClientHandler. So this listener handles the + // error case, when the channel is closed and the NettyClientHandler no longer in the pipeline. ChannelFutureListener failureListener = new ChannelFutureListener() { @Override public void operationComplete(ChannelFuture future) throws Exception { @@ -238,13 +240,11 @@ public Attributes getAttrs() { private Status statusFromFailedFuture(ChannelFuture f) { Throwable t = f.cause(); if (t instanceof ClosedChannelException) { - synchronized (this) { - Status shutdownStatus = lifecycleManager.getShutdownStatus(); - if (shutdownStatus == null) { - return Status.UNKNOWN.withDescription("Channel closed but for unknown reason"); - } - return shutdownStatus; + Status shutdownStatus = lifecycleManager.getShutdownStatus(); + if (shutdownStatus == null) { + return Status.UNKNOWN.withDescription("Channel closed but for unknown reason"); } + return shutdownStatus; } return Utils.statusFromThrowable(t); } diff --git a/netty/src/main/java/io/grpc/netty/Utils.java b/netty/src/main/java/io/grpc/netty/Utils.java index e68f693ba46..1b8c47c354a 100644 --- a/netty/src/main/java/io/grpc/netty/Utils.java +++ b/netty/src/main/java/io/grpc/netty/Utils.java @@ -158,10 +158,7 @@ public static Status statusFromThrowable(Throwable t) { if (s.getCode() != Status.Code.UNKNOWN) { return s; } - // TODO(ejona): reenable once startup races are resolved; ClosedChannelException is being seen - // still. Some tests are asserting UNAVAILABLE and were "working" previously but are now - // detecting that our behavior is flaky. See #1330 - if (false && t instanceof ClosedChannelException) { + if (t instanceof ClosedChannelException) { // ClosedChannelException is used any time the Netty channel is closed. Proper error // processing requires remembering the error that occurred before this one and using it // instead. diff --git a/testing/src/main/java/io/grpc/internal/testing/AbstractTransportTest.java b/testing/src/main/java/io/grpc/internal/testing/AbstractTransportTest.java index 6e83ee14c33..a501a80af10 100644 --- a/testing/src/main/java/io/grpc/internal/testing/AbstractTransportTest.java +++ b/testing/src/main/java/io/grpc/internal/testing/AbstractTransportTest.java @@ -52,7 +52,6 @@ import static org.mockito.Mockito.timeout; import static org.mockito.Mockito.verify; -import com.google.common.base.Throwables; import com.google.common.collect.Lists; import com.google.common.io.ByteStreams; import com.google.common.util.concurrent.MoreExecutors; @@ -459,13 +458,7 @@ public void ping_afterTermination() throws Exception { } verify(mockPingCallback, timeout(TIMEOUT_MS)).onFailure(throwableCaptor.capture()); Status status = Status.fromThrowable(throwableCaptor.getValue()); - // TODO(buchgr): Remove once https://github.com/grpc/grpc-java/issues/1330 is resolved. - String stackTrace = ""; - if (Status.UNAVAILABLE.getCode() != status.getCode() - && status.getCause() != null) { - stackTrace = Throwables.getStackTraceAsString(status.getCause()); - } - assertCodeEquals(stackTrace, Status.UNAVAILABLE, status); + assertCodeEquals(Status.UNAVAILABLE, status); } @Test From 7fb5bef00dfdc96693d408936dc50ff6a356a0ff Mon Sep 17 00:00:00 2001 From: buchgr Date: Mon, 12 Sep 2016 18:55:55 +0200 Subject: [PATCH 2/2] Address comments. --- netty/src/main/java/io/grpc/netty/NettyClientHandler.java | 4 ++++ netty/src/main/java/io/grpc/netty/NettyClientTransport.java | 2 ++ 2 files changed, 6 insertions(+) diff --git a/netty/src/main/java/io/grpc/netty/NettyClientHandler.java b/netty/src/main/java/io/grpc/netty/NettyClientHandler.java index 7b9ccffa9a7..3810a0acf31 100644 --- a/netty/src/main/java/io/grpc/netty/NettyClientHandler.java +++ b/netty/src/main/java/io/grpc/netty/NettyClientHandler.java @@ -476,6 +476,10 @@ public void operationComplete(ChannelFuture future) throws Exception { 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) { diff --git a/netty/src/main/java/io/grpc/netty/NettyClientTransport.java b/netty/src/main/java/io/grpc/netty/NettyClientTransport.java index bd036ffb2c7..9cec1fa07e7 100644 --- a/netty/src/main/java/io/grpc/netty/NettyClientTransport.java +++ b/netty/src/main/java/io/grpc/netty/NettyClientTransport.java @@ -236,6 +236,8 @@ public Attributes getAttrs() { * Convert ChannelFuture.cause() to a Status, taking into account that all handlers are removed * from the pipeline when the channel is closed. Since handlers are removed, you may get an * unhelpful exception like ClosedChannelException. + * + *

This method must only be called on the event loop. */ private Status statusFromFailedFuture(ChannelFuture f) { Throwable t = f.cause();