-
Notifications
You must be signed in to change notification settings - Fork 641
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
Ensure IdleTimeoutHandler
receives always channelRead
events
#2660
Conversation
- When the server is configured with HTTP/1.1 and H2C ensure Http2MultiplexHandler is at the end of the pipeline because this handler doesn't forward all channelRead events. - When the server is configured with HTTP/1.1 and H2 and the client sends HTTP/1.1, when channelActive event happens, HttpTrafficHandler is still not in the pipeline. In this use case add IdleTimeoutHandler when HttpTrafficHandler is added to the pipeline. Fixes #2649
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.
Looks good to me.
One minor thing: it seems that in HttpTrafficHandler.channelActive
method line 138, the IdleTimeoutHandler.addIdleTimeoutHandler(ctx.pipeline(), idleTimeout);
call can be removed
…r is added at the point when HTTP/1.1 is negotiated Previously it was added with HttpTrafficHandler#handlerAdded, for this use case this is correct, but for the other use cases we don't want to add IdleTimeoutHandler at this point because we don't want this handler to participate in the TLS handshake.
On a second thought, although that |
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.
+1
@pderop Thanks for the review! |
HTTP/1.1
andH2C
ensureHttp2MultiplexHandler
is at the end of the pipeline because this handler doesn't forward allchannelRead
events.HTTP/1.1
andH2
and the client sendsHTTP/1.1
, whenchannelActive
event happens,HttpTrafficHandler
is still not in the pipeline. In this use case addIdleTimeoutHandler
whenHttpTrafficHandler
is added to the pipeline.Fixes #2649