From b364edff304bb0735ccc4b19097e3dd22122d5fe Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 30 Jul 2021 13:24:05 -0700 Subject: [PATCH] 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 --- .../main/java/io/grpc/netty/NettyClientHandler.java | 10 ++++++---- .../java/io/grpc/netty/NettyClientHandlerTest.java | 11 ++++++++++- 2 files changed, 16 insertions(+), 5 deletions(-) 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));