netty: Remove connectionError and just use goAwayStatus

ejona86 · ejona86 · commit 017cdd2ae99b · 2015-08-10T13:23:27.000-07:00
When connectionError was set, goAwayStatus was also set, so we shouldn't
lose any errors.

NettyClientTransport doesn't really need a Throwable, it just needs a
Status. Passing a Status out of NettyClientHandler reduces the number of
places that need to do transport-specific translation of Throwables into
Status codes.
diff --git a/netty/src/main/java/io/grpc/transport/netty/NettyClientHandler.java b/netty/src/main/java/io/grpc/transport/netty/NettyClientHandler.java
@@ -88,7 +88,6 @@ class NettyClientHandler extends Http2ConnectionHandler {
   private WriteQueue clientWriteQueue;
   private int flowControlWindow;
   private Http2Settings initialSettings = new Http2Settings();
-  private Throwable connectionError;
   private Http2Ping ping;
   private Status goAwayStatus;
   private ChannelHandlerContext ctx;
@@ -132,8 +131,8 @@ public void onGoAwayReceived(int lastStreamId, long errorCode, ByteBuf debugData
   }
 
   @Nullable
-  public Throwable connectionError() {
-    return connectionError;
+  public Status errorStatus() {
+    return goAwayStatus;
   }
 
   @Override
@@ -253,9 +252,7 @@ protected void onConnectionError(ChannelHandlerContext ctx, Throwable cause,
       Http2Exception http2Ex) {
     logger.log(Level.FINE, "Caught a connection error", cause);
 
-    // Save the error.
-    connectionError = cause;
-    goAwayStatus(Status.fromThrowable(connectionError));
+    goAwayStatus(Status.fromThrowable(cause));
     cancelPing();
 
     super.onConnectionError(ctx, cause, http2Ex);
@@ -429,9 +426,7 @@ private void cancelPing() {
   }
 
   private Throwable getPingFailure() {
-    if (connectionError != null) {
-      return connectionError;
-    } else if (goAwayStatus != null) {
+    if (goAwayStatus != null) {
       return goAwayStatus.asException();
     } else {
       return Status.UNAVAILABLE.withDescription("Connection closed").asException();
diff --git a/netty/src/main/java/io/grpc/transport/netty/NettyClientTransport.java b/netty/src/main/java/io/grpc/transport/netty/NettyClientTransport.java
@@ -65,17 +65,13 @@
 import java.net.InetSocketAddress;
 import java.net.SocketAddress;
 import java.util.concurrent.Executor;
-import java.util.logging.Level;
-import java.util.logging.Logger;
 
 import javax.annotation.concurrent.GuardedBy;
 
 /**
  * A Netty-based {@link ClientTransport} implementation.
  */
 class NettyClientTransport implements ClientTransport {
-  private static final Logger log = Logger.getLogger(NettyClientTransport.class.getName());
-
   private final SocketAddress address;
   private final Class<? extends Channel> channelType;
   private final EventLoopGroup group;
@@ -183,34 +179,36 @@ public void operationComplete(ChannelFuture future) throws Exception {
         } else {
           // Need to notify of this failure, because handler.connectionError() is not guaranteed to
           // have seen this cause.
-          notifyTerminated(future.cause());
+          notifyTerminated(Status.fromThrowable(future.cause()));
         }
       }
     });
     // Handle transport shutdown when the channel is closed.
     channel.closeFuture().addListener(new ChannelFutureListener() {
       @Override
       public void operationComplete(ChannelFuture future) throws Exception {
-        notifyTerminated(handler.connectionError());
+        Status status = handler.errorStatus();
+        if (status == null) {
+          // We really only expect this to happen if shutdown() was called, but in that case this
+          // status is ignored.
+          status = Status.INTERNAL.withDescription("Connection closed with unknown cause");
+        }
+        notifyTerminated(status);
       }
     });
   }
 
   @Override
   public void shutdown() {
-    notifyShutdown(null);
+    notifyShutdown(Status.OK.withDescription("Channel requested transport to shut down"));
     // Notifying of termination is automatically done when the channel closes.
     if (channel != null && channel.isOpen()) {
       channel.close();
     }
   }
 
-  private void notifyShutdown(Throwable t) {
-    Status s = Status.OK.withDescription("Transport is shutting down");
-    if (t != null) {
-      log.log(Level.SEVERE, "Transport failed", t);
-      s = Status.INTERNAL.withCause(t).withDescription("Transport failed");
-    }
+  private void notifyShutdown(Status status) {
+    Preconditions.checkNotNull(status, "status");
     boolean notifyShutdown;
     synchronized (this) {
       notifyShutdown = !shutdown;
@@ -219,12 +217,12 @@ private void notifyShutdown(Throwable t) {
       }
     }
     if (notifyShutdown) {
-      listener.transportShutdown(s);
+      listener.transportShutdown(status);
     }
   }
 
-  private void notifyTerminated(Throwable t) {
-    notifyShutdown(t);
+  private void notifyTerminated(Status status) {
+    notifyShutdown(status);
     boolean notifyTerminated;
     synchronized (this) {
       notifyTerminated = !terminated;
diff --git a/netty/src/test/java/io/grpc/transport/netty/NettyClientHandlerTest.java b/netty/src/test/java/io/grpc/transport/netty/NettyClientHandlerTest.java
@@ -522,11 +522,11 @@ public void ping_failsOnConnectionError() throws Exception {
     sendPing(callback, MoreExecutors.directExecutor());
     assertEquals(0, callback.invocationCount);
 
-    Throwable connectionError = new Throwable();
-    handler.onConnectionError(ctx, connectionError, null);
+    Status failureStatus = Status.INTERNAL.withDescription("forced failure");
+    handler.onConnectionError(ctx, failureStatus.asRuntimeException(), null);
     // ping failed on connection error
     assertEquals(1, callback.invocationCount);
-    assertSame(connectionError, callback.failureCause);
+    assertSame(failureStatus, Status.fromThrowable(callback.failureCause));
   }
 
   private void sendPing(PingCallback callback, Executor executor) {
