netty: Refine workaround for Netty header processing for transparent …

…retries

Nginx and C core don't do graceful GOAWAY and retries have matured such
that transparent retries may soon be on by default. Refining the
workaround thus can reduces error rate for users.

Fixes #8310

netty/src/main/java/io/grpc/netty/NettyClientHandler.java

@@ -822,6 +822,7 @@ private void goingAway(long errorCode, byte[] debugData) {
     // UNAVAILABLE. https://github.com/netty/netty/issues/10670
     final Status abruptGoAwayStatusConservative = statusFromH2Error(
         null, "Abrupt GOAWAY closed sent stream", errorCode, debugData);
+    final boolean mayBeHittingNettyBug = errorCode != Http2Error.NO_ERROR.code();
     // Try to allocate as many in-flight streams as possible, to reduce race window of
     // https://github.com/grpc/grpc-java/issues/2562 . To be of any help, the server has to
     // gracefully shut down the connection with two GOAWAYs. gRPC servers generally send a PING
@@ -848,11 +849,12 @@ public boolean visit(Http2Stream stream) throws Http2Exception {
             if (clientStream != null) {
               // RpcProgress _should_ be REFUSED, but are being conservative. See comment for
               // abruptGoAwayStatusConservative. This does reduce our ability to perform transparent
-              // retries, but our main goal of transporent retries is to resolve the local race. We
-              // still hope/expect servers to use the graceful double-GOAWAY when closing
-              // connections.
+              // retries, but only if something else caused a connection failure.
+              RpcProgress progress = mayBeHittingNettyBug
+                  ? RpcProgress.PROCESSED
+                  : RpcProgress.REFUSED;
               clientStream.transportReportStatus(
-                  abruptGoAwayStatusConservative, RpcProgress.PROCESSED, false, new Metadata());
+                  abruptGoAwayStatusConservative, progress, false, new Metadata());
             }
             stream.close();
           }

netty/src/test/java/io/grpc/netty/NettyClientHandlerTest.java

@@ -331,9 +331,18 @@ public void inboundShouldForwardToStream() throws Exception {
   }
 
   @Test
-  public void receivedGoAwayShouldRefuseLaterStreamId() throws Exception {
+  public void receivedGoAwayNoErrorShouldRefuseLaterStreamId() throws Exception {
     ChannelFuture future = enqueue(newCreateStreamCommand(grpcHeaders, streamTransportState));
     channelRead(goAwayFrame(streamId - 1));
+    verify(streamListener).closed(any(Status.class), eq(REFUSED), any(Metadata.class));
+    assertTrue(future.isDone());
+  }
+
+  @Test
+  public void receivedGoAwayErrorShouldRefuseLaterStreamId() throws Exception {
+    ChannelFuture future = enqueue(newCreateStreamCommand(grpcHeaders, streamTransportState));
+    channelRead(
+        goAwayFrame(streamId - 1, (int) Http2Error.PROTOCOL_ERROR.code(), Unpooled.EMPTY_BUFFER));
     // This _should_ be REFUSED, but we purposefully use PROCESSED. See comment for
     // abruptGoAwayStatusConservative in NettyClientHandler
     verify(streamListener).closed(any(Status.class), eq(PROCESSED), any(Metadata.class));