Skip to content

Commit 2a766c7

Browse files
committed
Guarantee missing stream promise delivery
In observed cases, whether RST_STREAM or another failure from netty or the server, listeners can fail to be notified when a connection yields a null stream for the selected streamId. This causes hangs in clients, despite deadlines, with no obvious resolution. Tests which relied upon this promise succeeding must now change.
1 parent 5a8326f commit 2a766c7

File tree

2 files changed

+14
-9
lines changed

2 files changed

+14
-9
lines changed

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

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -738,14 +738,19 @@ public void operationComplete(ChannelFuture future) throws Exception {
738738

739739
// Attach the client stream to the HTTP/2 stream object as user data.
740740
stream.setHttp2Stream(http2Stream);
741+
promise.setSuccess();
742+
} else {
743+
// Otherwise, the stream has been cancelled and Netty is sending a
744+
// RST_STREAM frame which causes it to purge pending writes from the
745+
// flow-controller and delete the http2Stream. The stream listener has already
746+
// been notified of cancellation so there is nothing to do.
747+
//
748+
// This process has been observed to fail in some circumstances, leaving listeners
749+
// unanswered. Ensure that some exception has been delivered consistent with the
750+
// implied RST_STREAM result above.
751+
Status status = Status.INTERNAL.withDescription("unknown stream for connection");
752+
promise.setFailure(status.asRuntimeException());
741753
}
742-
// Otherwise, the stream has been cancelled and Netty is sending a
743-
// RST_STREAM frame which causes it to purge pending writes from the
744-
// flow-controller and delete the http2Stream. The stream listener has already
745-
// been notified of cancellation so there is nothing to do.
746-
747-
// Just forward on the success status to the original promise.
748-
promise.setSuccess();
749754
} else {
750755
Throwable cause = future.cause();
751756
if (cause instanceof StreamBufferingEncoder.Http2GoAwayException) {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,7 @@ public void cancelBufferedStreamShouldChangeClientStreamStatus() throws Exceptio
268268
// Cancel the stream.
269269
cancelStream(Status.CANCELLED);
270270

271-
assertTrue(createFuture.isSuccess());
271+
assertFalse(createFuture.isSuccess());
272272
verify(streamListener).closed(eq(Status.CANCELLED), same(PROCESSED), any(Metadata.class));
273273
}
274274

@@ -311,7 +311,7 @@ public void cancelWhileBufferedShouldSucceed() throws Exception {
311311
ChannelFuture cancelFuture = cancelStream(Status.CANCELLED);
312312
assertTrue(cancelFuture.isSuccess());
313313
assertTrue(createFuture.isDone());
314-
assertTrue(createFuture.isSuccess());
314+
assertFalse(createFuture.isSuccess());
315315
}
316316

317317
/**

0 commit comments

Comments
 (0)