-
-
Notifications
You must be signed in to change notification settings - Fork 15.8k
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.
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
Proper shutdown of HTTP2 encoder when channelInactive #3961
Conversation
Motivation: The problem is described in grpc/grpc-java#605. Basically, when using `StreamBufferingEncoder` there is a chance of creating zombie streams that never get closed. Modifications: Change `Http2ConnectionHandler`'s `channelInactive` handling logic to shutdown the encoder/decoder before shutting down the active streams. Result: Fixes grpc/grpc-java#605
@Scottmitch @louiscryan PTAL |
* Thrown if buffered streams are terminated due to this encoder being closed. | ||
*/ | ||
public static final class ChannelClosedException extends Http2Exception { | ||
public ChannelClosedException() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add serialVersionUID
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
@nmittler - Just 1 small comment...and fix build failure...then lgtm. |
@@ -127,6 +139,10 @@ public ChannelFuture writeHeaders(ChannelHandlerContext ctx, int streamId, Http2 | |||
public ChannelFuture writeHeaders(ChannelHandlerContext ctx, int streamId, Http2Headers headers, | |||
int streamDependency, short weight, boolean exclusive, | |||
int padding, boolean endOfStream, ChannelPromise promise) { | |||
if (closed) { | |||
promise.setFailure(new ChannelClosedException()); | |||
return promise; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merge the two lines... return promise.setFailure(....)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
@normanmaurer anything else or shall I cherry-pick? |
@nmittler - LGTM! |
Motivation:
The problem is described in grpc/grpc-java#605. Basically, when using
StreamBufferingEncoder
there is a chance of creating zombie streams that never get closed.Modifications:
Change
Http2ConnectionHandler
'schannelInactive
handling logic to shutdown the encoder/decoder before shutting down the active streams.Result:
Fixes grpc/grpc-java#605