Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

netty: Refine workaround for Netty header processing for transparent retries #8359

Merged
merged 1 commit into from Jul 30, 2021
Merged

netty: Refine workaround for Netty header processing for transparent retries #8359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 6 additions & 4 deletions netty/src/main/java/io/grpc/netty/NettyClientHandler.java
View file
Open in desktop
Expand Up @@ -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
Expand All @@ -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();
}
Expand Down
11 changes: 10 additions & 1 deletion netty/src/test/java/io/grpc/netty/NettyClientHandlerTest.java
View file
Open in desktop
Expand Up @@ -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));
Expand Down