Skip to content

Fix an NPE in AbstractHttp2StreamChannel#9379

Merged
normanmaurer merged 2 commits intonetty:4.1from
bryce-anderson:banderson/issue9375-NPE
Jul 17, 2019
Merged

Fix an NPE in AbstractHttp2StreamChannel#9379
normanmaurer merged 2 commits intonetty:4.1from
bryce-anderson:banderson/issue9375-NPE

Conversation

@bryce-anderson
Copy link
Copy Markdown
Contributor

Motivation:

If a read triggers a AbstractHttp2StreamChannel to close we can
get an NPE in the read loop.

Modifications:

Make sure that the inboundBuffer isn't null before attempting to
continue the loop.

Result:

Fixes #9375.
No NPE.

Motivation:

If a read triggers a AbstractHttp2StreamChannel to close we can
get an NPE in the read loop.

Modifications:

Make sure that the inboundBuffer isn't null before attempting to
continue the loop.

Result:

No NPE.
Fixes netty#9375.
@netty-bot
Copy link
Copy Markdown

Can one of the admins verify this patch?

@bryce-anderson
Copy link
Copy Markdown
Contributor Author

I don't know how to write a test for this, so pointers appreciated (no pun intended). It also feels like something that we shouldn't necessarily need a test for.

@normanmaurer
Copy link
Copy Markdown
Member

@bryce-anderson let me see if I can write one..

@normanmaurer normanmaurer added this to the 4.1.38.Final milestone Jul 17, 2019
@normanmaurer
Copy link
Copy Markdown
Member

@bryce-anderson please check 45136e6 ... This is the unit test for the NPE :)

@normanmaurer
Copy link
Copy Markdown
Member

@netty-bot test this please

@normanmaurer normanmaurer merged commit dd1785b into netty:4.1 Jul 17, 2019
@normanmaurer
Copy link
Copy Markdown
Member

@bryce-anderson thanks a lot... can you verify if finagle tests now pass with this pulled in ?

normanmaurer pushed a commit that referenced this pull request Jul 17, 2019
Motivation:

If a read triggers a AbstractHttp2StreamChannel to close we can
get an NPE in the read loop.

Modifications:

Make sure that the inboundBuffer isn't null before attempting to
continue the loop.

Result:

No NPE.
Fixes #9337
@bryce-anderson
Copy link
Copy Markdown
Contributor Author

I tried the patch based on 4.1.37.Final and it seems to have fixed it.

@normanmaurer
Copy link
Copy Markdown
Member

@bryce-anderson so everything is good now ? Which means if we ship a new release with the fix you can upgrade ?

@bryce-anderson
Copy link
Copy Markdown
Contributor Author

Well, when I tried against 4.1.38.Final-SNAPSHOT something else wasn't working, but I don't know what and spent 0 time looking into it, so kinda...

@bryce-anderson
Copy link
Copy Markdown
Contributor Author

I'm also digging into a problem where I see HTTP2 data packets getting pushed down the stream pipelines out of order (I get a eos packet that skips the stuff in inboundBuffer) but I don't have anything ticket-worthy right now so I haven't filed one. That is one that we'll really need.

@normanmaurer
Copy link
Copy Markdown
Member

@bryce-anderson can you check if all works with 4.1.36.Final only ? We need to find a way to track all of this down

@bryce-anderson
Copy link
Copy Markdown
Contributor Author

Let's start a ticket for the out-of-order thing. It is also broken in 4.1.35.Final, and I may have just tracked it down (maybe not).

@bryce-anderson
Copy link
Copy Markdown
Contributor Author

Opened #9387.

Copy link
Copy Markdown
Member

@carl-mastrangelo carl-mastrangelo left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NullPointerException in AbstractHttp2StreamChannel

4 participants