Skip to content

HttpContentDecoder must continue read when it did not produce any mes… #8922

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

Merged
merged 2 commits into from
Mar 7, 2019

Conversation

normanmaurer
Copy link
Member

…sage and auto read is false

Motivation:

When HttpContentDecoder (and so HttpContentDecompressor) does not produce any message we need to make sure it calls ctx.read() if auto read is false to not stale.

Modifications:

  • Keep track if we need to call ctx.read() or not
  • Add unit test

Result:

Fixes #8915.

…sage and auto read is false

Motivation:

When HttpContentDecoder (and so HttpContentDecompressor) does not produce any message we need to make sure it calls ctx.read() if auto read is false to not stale.

Modifications:

- Keep track if we need to call ctx.read() or not
- Add unit test

Result:

Fixes #8915.
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

Why isn't this part of MessageToMessageDecoder?

try {
decodeContent(c, out);
} finally {
if (!needRead) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this variable handling is inverted. needRead should start as true, and if !out.isEmpty() it should be set to false. Then in channelReadComplete() it should be set to true again.

If a read() causes three ByteBufs to be read (because using DefaultMaxBytesRecvByteBufAllocator), which calls decode() three times, the first two times may not put anything in out but the last decode does place something in out. It seems in that case we shouldn't call read.

But there's also the case where the second bytebuf placed something in out. That seems like it shouldn't call read either.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ejona86 sure why not.

@normanmaurer
Copy link
Member Author

@ejona86 its not mark of MessageToMessageDecoder because we messed in the past and allowed it to be @Sharable. In the next major release we should fix this but there is nothing we can do there for now :(

@andreisilviudragnea
Copy link

I can confirm too that #8915 is fixed by these changes. I have also understood now how the Inflater used behind the scenes by JdkZlibDecoder decompresses as much as it can from the input buffer, passing the output on. Initially, I thought JdkZlibDecoder also uses the accumulator buffer approach like the ByteToMessageDecoder Thank you for the quick fix! 😄

@normanmaurer normanmaurer merged commit 67663fa into 4.1 Mar 7, 2019
@normanmaurer normanmaurer deleted the decompressor_read branch March 7, 2019 09:31
normanmaurer added a commit that referenced this pull request Mar 7, 2019
#8922)

Motivation:

When HttpContentDecoder (and so HttpContentDecompressor) does not produce any message we need to make sure it calls ctx.read() if auto read is false to not stale.

Modifications:

- Keep track if we need to call ctx.read() or not
- Add unit test

Result:

Fixes #8915.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants