diff --git a/netty/src/main/java/io/grpc/netty/NettyClientHandler.java b/netty/src/main/java/io/grpc/netty/NettyClientHandler.java index d263356204e..22d8fcadb75 100644 --- a/netty/src/main/java/io/grpc/netty/NettyClientHandler.java +++ b/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(); } diff --git a/netty/src/test/java/io/grpc/netty/NettyClientHandlerTest.java b/netty/src/test/java/io/grpc/netty/NettyClientHandlerTest.java index 5f1d27c37e2..d0d48fe9b48 100644 --- a/netty/src/test/java/io/grpc/netty/NettyClientHandlerTest.java +++ b/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));