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.

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

Buffer Messages until TLS Handshake and HTTP2 Negotiation complete #116

Closed
buchgr opened this issue Feb 24, 2015 · 2 comments
Closed

Buffer Messages until TLS Handshake and HTTP2 Negotiation complete #116

buchgr opened this issue Feb 24, 2015 · 2 comments
Labels

Comments

@buchgr
Copy link
Collaborator

buchgr commented Feb 24, 2015

When grpc uses Netty as the client transport all RPC calls (aka HTTP2 Streams) block until the TLS Handshake and the HTTP2 negotiation is complete.

This blocking implementation (in grpc) is currently required as Netty's SslHandler doesn't buffer messages until the Handshake is complete ("You must make sure not to write a message while the handshake is in progress unless you are renegotiating."), and there is nothing to stop the user from starting to make RPC calls immediately.

This behavior comes with two problems:

  1. With RPC calls blocking until the TLS Handshake is complete, every call launched before the TLS Handshake and HTTP2 Negotiation are done will block its thread from which one would expect async behavior though.
  2. In cases when a DirectExecutor is being used it might lead to the EventLoop blocking forever (deadlock effectively). There is several scenarios how a deadlock could happen. One such scenario is when you are writing a server in Netty and within that server you want to connect to a grpc service to fetch some data. If you now use a DirectExecutor and reuse the EventLoop of the server with the grpc client, the TLS handshake would block the server's EventLoop, which is also the very EventLoop responsible for completing the TLS HandShake. That way neither the server nor the client would ever make progress again.

@nmittler , @ejona86 and I talked about this problem earlier today and we agreed to get rid of the blocking behavior by adding an additional ChannelHandler to the end of the pipeline (tail) that will buffer any data until TLS & HTTP2 are working. After that it will send the buffered messages through the pipeline and remove itself from the pipeline.

@nmittler @ejona86 @louiscryan

@rschildmeijer
Copy link

...once again.
Buffering the outstanding streams might be ok until you run into OOM issues.

"and there is nothing to stop the user from starting to make RPC calls immediately."
could be worth exposing a way to block on the ssl handshake. Something like a .sync()[1] or .get()[2] call.

[1] http://netty.io/4.0/api/io/netty/util/concurrent/Future.html#sync%28%29
[2] http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Future.html#get%28%29

@ejona86
Copy link
Member

ejona86 commented Feb 26, 2015

@rschildmeijer, we would delay the call to onReady that is being done as part of #6. If using the blocking stub, you would naturally block. The async stub would still be able to return immediately as its callers would generally expect.

@ejona86 ejona86 added the netty label Mar 11, 2015
buchgr added a commit to buchgr/grpc-java that referenced this issue Apr 30, 2015
Motivation:

We are currently blocking in NettyClientTransport.newStream(...) until the channel is active and/or the TLS Handshake is complete.
In certain cases this may lead to deadlock of the eventloop, see grpc#116 for details.

Modifications:

Remove all blocking by buffering writes until the channel is ready to receive those i.e. it is active, TLS is negotiated or the HTTP to HTTP/2 upgrade was sucessfull.

Result:

No more blocking parts when using Netty on the client side.
buchgr added a commit to buchgr/grpc-java that referenced this issue Apr 30, 2015
Motivation:

We are currently blocking in NettyClientTransport.newStream(...) until the channel is active and/or the TLS Handshake is complete.
In certain cases this may lead to deadlock of the eventloop, see grpc#116 for details.

Modifications:

Remove all blocking by buffering writes until the channel is ready to receive those i.e. it is active, TLS is negotiated or the HTTP to HTTP/2 upgrade was sucessfull.

Result:

No more blocking parts when using Netty on the client side.
buchgr added a commit to buchgr/grpc-java that referenced this issue May 1, 2015
Motivation:

We are currently blocking in NettyClientTransport.newStream(...) until the channel is active and/or the TLS Handshake is complete.
In certain cases this may lead to deadlock of the eventloop, see grpc#116 for details.

Modifications:

Remove all blocking by buffering writes until the channel is ready to receive those i.e. it is active, TLS is negotiated or the HTTP to HTTP/2 upgrade was sucessfull.

Result:

No more blocking parts when using Netty on the client side.
@ejona86 ejona86 closed this as completed in 1af367c May 4, 2015
@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.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants