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

BufferingHttp2ConnectionEncoder does not shutdown properly on channelInactive #605

Closed
louiscryan opened this issue Jul 8, 2015 · 2 comments
Closed

BufferingHttp2ConnectionEncoder does not shutdown properly on channelInactive #605

louiscryan opened this issue Jul 8, 2015 · 2 comments

Comments

@louiscryan
Copy link
Contributor

@nmittler

There is a nasty race condition during the handling of channelInactive in NettyClientHandler which goes a bit like this....

  1. NettyClientHandler.channelInactive -> for each active stream report closure to GRPC
  2. NettyClientHandler.channelInactive -> Http2ConnectionHandler.channelInactive -> Http2ConnectionHandler.BaseDecoder.channelInactive -> for each active stream call close -> BufferingHttp2ConnectionEncoder.Http2ConnectionAdapter.onStreamClose -> try creating new stream -> adds stream to active list (OOPS! this stream is never closed)

This reproduces for NettyClientTransportTest.bufferedStreamsShouldBeClosedWhenTransportTerminates with 5.0beta5.

Having streams being created as a side-effect of channel inactivation is undesirable. Potential fixes include

  • Reorder teardown in Http2ConnectionHandler.BaseDecoder.channelInactive so encoders are closed() before streams are closed.
  • Make BufferedHttp2ConnectionEncoder check channel.isActive() when trying to create streams.
@nmittler
Copy link
Member

nmittler commented Jul 8, 2015

@louiscryan I've already been working on a fix for this. See #590

nmittler pushed a commit to nmittler/netty that referenced this issue Jul 8, 2015
Proper shutdown of HTTP2 encoder when channelInactive
7ebc11e 
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
This was referenced Jul 8, 2015
Closed
Closed
nmittler pushed a commit to netty/netty that referenced this issue Jul 9, 2015
Proper shutdown of HTTP2 encoder when channelInactive
6e044b0 
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
@nmittler
Copy link
Member

nmittler commented Jul 9, 2015

NOTE: this has been fixed in Netty. We'll get the fix when we upgrade to Netty 4.1.beta6 (as soon as it is released).

@normanmaurer any thoughts on timeline for beta6?

@lock lock bot locked as resolved and limited conversation to collaborators Sep 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@nmittler @louiscryan