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

NullPointerException during Client Side ClearText Upgrade with new Http2MultiplexHandler.java #9495

Closed
nizarm opened this issue Aug 22, 2019 · 2 comments
Closed

NullPointerException during Client Side ClearText Upgrade with new Http2MultiplexHandler.java #9495

nizarm opened this issue Aug 22, 2019 · 2 comments

Comments

@nizarm
Copy link
Contributor

nizarm commented Aug 22, 2019

When cleartext upgrade is implemented in client side using HttpClientUpgradeHandler there is no way to pass the newly created Http2MultiplexHandler.

It seems like we need a way to correctly add Http2MultiplexHandler to the pipeline before calling Http2FrameCodec.onHttpClientUpgrade(...). Since this special case is not handled in HttpClientUpgradeHandler (but seems like this case is correctly taken care in Http2ServerUpgradeCodec). 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.

Following is the exception I am receiving when I setup the Http2MultiplexHandler after the upgrade is taken place

va.lang.NullPointerException
at io.netty.handler.codec.http2.AbstractHttp2StreamChannel$1.visit(AbstractHttp2StreamChannel.java:64) ~[netty-all-4.1.39.Final.jar:4.1.39.Final]
at io.netty.handler.codec.http2.Http2FrameCodec$2.visit(Http2FrameCodec.java:200) ~[netty-all-4.1.39.Final.jar:4.1.39.Final]
at io.netty.handler.codec.http2.DefaultHttp2Connection$ActiveStreams.forEachActiveStream(DefaultHttp2Connection.java:971) ~[netty-all-4.1.39.Final.jar:4.1.39.Final]
at io.netty.handler.codec.http2.DefaultHttp2Connection.forEachActiveStream(DefaultHttp2Connection.java:208) ~[netty-all-4.1.39.Final.jar:4.1.39.Final]
at io.netty.handler.codec.http2.Http2FrameCodec.forEachActiveStream(Http2FrameCodec.java:196) ~[netty-all-4.1.39.Final.jar:4.1.39.Final]
at io.netty.handler.codec.http2.Http2ChannelDuplexHandler.forEachActiveStream(Http2ChannelDuplexHandler.java:83) ~[netty-all-4.1.39.Final.jar:4.1.39.Final]
at io.netty.handler.codec.http2.Http2MultiplexHandler.channelWritabilityChanged(Http2MultiplexHandler.java:199) ~[netty-all-4.1.39.Final.jar:4.1.39.Final]
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelWritabilityChanged(AbstractChannelHandlerContext.java:436) ~[netty-all-4.1.39.Final.jar:4.1.39.Final]

My Code looks something like this

    Http2ClientUpgradeCodec upgradeCodec = new Http2ClientUpgradeCodec(
        "Http2Codec",
        Http2FrameCodecBuilder
        .forClient()
        .initialSettings(createHttp2Settings())
        .build()
        );

    channel.pipeline().addLast(sourceCodec);
    channel.pipeline().addLast(new HttpClientUpgradeHandler(sourceCodec, upgradeCodec, _maxContentLength));
    channel.pipeline().addLast(new Http2ProtocolUpgradeHandler(upgradePromise));

I add the Http2MultiplexHandler inside my Http2ProtocolUpgradeHandler during success of cleartext upgrade (as I dont have any other way to add it along with upgradecodec).

Netty Version I used : 4.1.39

When I follow the same approach to fix the serverside upgrade issue as mentioned in
#9314 - it works perfectly form me.

@normanmaurer , @trustin - let me know if this is a known bug and we are good with my proposed fix. I can give a pull request !
@normanmaurer
Copy link
Member

@nizarm sounds like a bug yes... I would be happy to review a PR.

@nizarm nizarm mentioned this issue Aug 22, 2019
Merged
@nizarm
Copy link
Contributor Author

nizarm commented Aug 22, 2019

Thanks for confirming @normanmaurer - PR is ready for your review !

nizarm pushed a commit to nizarm/netty that referenced this issue Aug 22, 2019
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 pushed a commit that referenced this issue 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.
evanw555 pushed a commit to linkedin/rest.li that referenced this issue Dec 18, 2019
@nizarm
1. New Netty Http2 Channel Abstraction APIs we used to unify the Nett…
cc9ebef 
…y Channel Pipeline has been deprecated from Netty 4.1.37 onwards and refactored in to lightweight APIs. Fix the deprecated APIs before the larger rollout and upgrade to latest Netty 4.1.39

2. When the deprecated APIs are refactored in to new light weight APIs - Unfortunally Netty did miss to special handle the newly created APIS 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 it ended up with an NPE if a frame was dispatched to the upgrade stream later on.

I have created a Netty Pull Request (PR) in github to fix this issue and has been acknwoledged by Norman (One of the Netty maintainers). The details of the bug and the pull request can be found @ netty/netty#9495
For now - I have included the fixed Http2ClientUpgradeCodec in pegasus until the next Netty release is available - to help continue the rollout of this feature !
3. In the previous release - due to known issue with Http2 flowcontrol in Netty - we have disabled backpressure for Http2 streams. Since the flowcontrol is taken care in Netty 4.1.39 - this limitation has been addressed and the new Http2 code pipeline is fully functional now !
4. All  the  special handling of Http2Stream channel test cases has been reverted as the known issues has been taken care now. Now all the integration test cases works for both old and new code path without any special handling - :)

RB=1775062
BUG=SI-11340
G=si-core-reviewers
R=fcapponi,ssheng,bsoetarm,crzhang
A=fcapponi,bsoetarm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@nizarm @normanmaurer