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

Correctly handle client side http2 upgrades when Http2FrameCodec is used together with Http2MultiplexHandler (9495) #9501

Merged
merged 3 commits into from Aug 23, 2019
Merged

Correctly handle client side http2 upgrades when Http2FrameCodec is used together with Http2MultiplexHandler (9495) #9501

merged 3 commits into from Aug 23, 2019

Conversation

nizarm
Copy link
Contributor

@nizarm nizarm commented Aug 22, 2019

Correctly handle client side http2 upgrades when Http2FrameCodec is used together with Http2MultiplexHandler (#9495)

Motivation:

In the release (4.1.37) we introduced Http2MultiplexHandler as a
replacement of Http2MultiplexCodec. This did split the frame parsing from
the multiplexing to allow a more flexible way to handle frames and to make
the code cleaner. Unfortunally we did miss to special handle this in
Http2ClientUpgradeCodec and so did not correctly add Http2MultiplexHandler
to the pipeline before calling Http2FrameCodec.onHttpClientUpgrade(...).
This did lead to the situation that we did not correctly receive the event
on the Http2MultiplexHandler and so did not correctly created the
Http2StreamChannel for the upgrade stream. Because of this we ended up
with an NPE if a frame was dispatched to the upgrade stream later on.

Modifications:

- Correctly add Http2MultiplexHandler to the pipeline before calling Http2FrameCodec.onHttpClientUpgrade(...)

Result:

Fixes #9495

@netty-bot
Copy link

Can one of the admins verify this patch?

@normanmaurer
Copy link
Member

@nizarm thanks I will have a look... that said can you sign our ICLA in the meantime: https://netty.io/s/icla

normanmaurer
normanmaurer requested changes Aug 22, 2019
View reviewed changes
Copy link
Member

@normanmaurer normanmaurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check my comments... also please add a unit test.

codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ClientUpgradeCodec.java Show resolved Hide resolved
codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ClientUpgradeCodec.java Show resolved Hide resolved
@nizarm
Copy link
Contributor Author

nizarm commented Aug 22, 2019

@nizarm thanks I will have a look... that said can you sign our ICLA in the meantime: https://netty.io/s/icla

Thanks, I have already submitted ICLA.

@normanmaurer
Copy link
Member

@nizarm thanks a lot... let me know once you addressed my comments and I will have a look again.

Correctly handle client side http2 upgrades when Http2FrameCodec is …
fcecc4e 
…used together with Http2MultiplexHandler (netty#9495)

    Motivation:

    In the release (4.1.37) we introduced Http2MultiplexHandler as a
    replacement of Http2MultiplexCodec. This did split the frame parsing from
    the multiplexing to allow a more flexible way to handle frames and to make
    the code cleaner. Unfortunally we did miss to special handle this in
    Http2ClientUpgradeCodec and so did not correctly add Http2MultiplexHandler
    to the pipeline before calling Http2FrameCodec.onHttpClientUpgrade(...).
    This did lead to the situation that we did not correctly receive the event
    on the Http2MultiplexHandler and so did not correctly created the
    Http2StreamChannel for the upgrade stream. Because of this we ended up
    with an NPE if a frame was dispatched to the upgrade stream later on.

    Modifications:

    - Correctly add Http2MultiplexHandler to the pipeline before calling Http2FrameCodec.onHttpClientUpgrade(...)

    Result:

    Fixes netty#9495.
    Added Unit Test
@normanmaurer
Copy link
Member

@netty-bot test this please

@normanmaurer
Copy link
Member

@netty-bot test this please

@normanmaurer normanmaurer added this to the 4.1.40.Final milestone Aug 23, 2019
@normanmaurer
Copy link
Member

@netty-bot test this please

mosesn
mosesn approved these changes Aug 23, 2019
View reviewed changes
Copy link
Contributor

@mosesn mosesn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not so involved in http2 stuff right now, but this looks reasonable to me. for context, here's what we do in finagle: https://github.com/twitter/finagle/blob/7825077f3ee502349c2c9baad8452e480855ed5b/finagle-http2/src/main/scala/com/twitter/finagle/http2/exp/transport/UpgradeRequestHandler.scala#L79-L92

@normanmaurer
Copy link
Member

@mosesn thanks for the review and feedback :)

@nizarm
Copy link
Contributor Author

nizarm commented Aug 23, 2019

Thanks @normanmaurer and @mosesn for approving these changes. Since I dont have write access, who will help merge this change? The merging of this change is automatically taken care?

@normanmaurer normanmaurer merged commit 14e856a into netty:4.1 Aug 23, 2019
@normanmaurer
Copy link
Member

@nizarm thanks a lot! I just merged it...

normanmaurer pushed a commit that referenced this pull request Aug 23, 2019
@nizarm @normanmaurer
Correctly handle client side http2 upgrades when Http2FrameCodec …(94…
130522b 
…95) (#9501)

Motivation:

In the release (4.1.37) we introduced Http2MultiplexHandler as a
replacement of Http2MultiplexCodec. This did split the frame parsing from
the multiplexing to allow a more flexible way to handle frames and to make
the code cleaner. Unfortunally we did miss to special handle this in
Http2ClientUpgradeCodec and so did not correctly add Http2MultiplexHandler
to the pipeline before calling Http2FrameCodec.onHttpClientUpgrade(...).
This did lead to the situation that we did not correctly receive the event
on the Http2MultiplexHandler and so did not correctly created the
Http2StreamChannel for the upgrade stream. Because of this we ended up
with an NPE if a frame was dispatched to the upgrade stream later on.

Modifications:

- Correctly add Http2MultiplexHandler to the pipeline before calling Http2FrameCodec.onHttpClientUpgrade(...)

Result:

Fixes #9495.
@normanmaurer normanmaurer changed the title Correctly handle client side http2 upgrades when Http2FrameCodec …(9495) Correctly handle client side http2 upgrades when Http2FrameCodec is used together with Http2MultiplexHandler (9495) Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mosesn mosesn mosesn approved these changes

@normanmaurer normanmaurer normanmaurer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.1.41.Final
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nizarm @netty-bot @normanmaurer @mosesn