HTTP/2: Ensure newStream() is called only once per connection upgrade and the correct handler is used#9396
Conversation
|
@bryce-anderson I think this should fix your problem.. please verify. |
|
@netty-bot test this please |
… and the correct handler is used Motivation: 3062993 introduced some code change to move the responsibility of creating the stream for the upgrade to Http2FrameCodec. Unfortunaly this lead to the situation of having newStream().setStreamAndProperty(...) be called twice. Because of this we only ever saw the channelActive(...) on Http2StreamChannel but no other events as the mapping was replaced on the second newStream().setStreamAndProperty(...) call. Beside this we also did not use the correct handler for the upgrade stream in some cases Modifications: - Just remove the Http2FrameCodec.onHttpClientUpgrade() method and so let the base class handle all of it. The stream is created correctly as part of the ConnectionListener implementation of Http2FrameCodec already. - Consolidate logic of creating stream channels - Adjust unit test to capture the bug Result: Fixes #9395
62c9d04 to
a92a6b4
Compare
|
@bryce-anderson this took me a while to figure out completely... There were actually two problems. This PR fixes all of them and now finagle tests pass again here as well. Can you please verify ? |
|
@netty-bot test this please |
|
Looks like it works from my end! |
|
@bryce-anderson cool... please approve |
bryce-anderson
left a comment
There was a problem hiding this comment.
Looks a lot better.
| } | ||
|
|
||
| private static boolean isServer(ChannelHandlerContext ctx) { | ||
| return ctx.channel().parent() instanceof ServerChannel; |
There was a problem hiding this comment.
This feels pretty brittle. Would this be better as a protected member of Http2ChannelDuplexHandler which has access to the connection() through the frameCodec field?
There was a problem hiding this comment.
we could do this but I wanted to keep the handlers as much as separated as possible ...
| @Override | ||
| final void onHttp2StreamStateChanged(ChannelHandlerContext ctx, DefaultHttp2FrameStream stream) { | ||
|
|
||
| switch (stream.state()) { |
There was a problem hiding this comment.
This looks identical to the Http2MultiplexHandler code other than the type of AbstractHttp2StreamChannel created (Http2MultiplexCodecStreamChannel vs Http2MultiplexHandlerStreamChannel). Could that be lifted into Http2ChannelDuplexHandler?
There was a problem hiding this comment.
Maybe it's okay since the multiplex codec is deprecated and presumably will be removed at some point?
There was a problem hiding this comment.
yeah that was my thinking
|
@bryce-anderson so with this in does the whole finagle build pass ? |
|
@normanmaurer, yes, it looks like we're back in business. Thanks! |
|
@bryce-anderson ok cool...let me merge this one. |
… and the correct handler is used (#9396) Motivation: 3062993 introduced some code change to move the responsibility of creating the stream for the upgrade to Http2FrameCodec. Unfortunaly this lead to the situation of having newStream().setStreamAndProperty(...) be called twice. Because of this we only ever saw the channelActive(...) on Http2StreamChannel but no other events as the mapping was replaced on the second newStream().setStreamAndProperty(...) call. Beside this we also did not use the correct handler for the upgrade stream in some cases Modifications: - Just remove the Http2FrameCodec.onHttpClientUpgrade() method and so let the base class handle all of it. The stream is created correctly as part of the ConnectionListener implementation of Http2FrameCodec already. - Consolidate logic of creating stream channels - Adjust unit test to capture the bug Result: Fixes #9395
|
@normanmaurer, do you have an idea of when 4.1.38 will be released? |
|
@bryce-anderson tomorrow or Thursday. |
Motivation:
3062993 introduced some code change to move the responsibility of creating the stream for the upgrade to Http2FrameCodec. Unfortunaly this lead to the situation of having newStream().setStreamAndProperty(...) be called twice. Because of this we only ever saw the channelActive(...) on Http2StreamChannel but no other events as the mapping was replaced on the second newStream().setStreamAndProperty(...) call.
Beside this we also did not use the correct handler for the upgrade stream in some cases
Modifications:
Result:
Fixes #9395