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.

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

[#5892] Correct handle HttpMessage that is EOF terminated #5957

Closed
wants to merge 1 commit into from

Conversation

@normanmaurer
Copy link
Member

commented Oct 31, 2016

Motivation:

We need to ensure we not add the Transfer-Encoding header if the HttpMessage is EOF terminated.

Modifications:

Only add the Transfer-Encoding header if an Content-Length header is present.

Result:

Correctly handle HttpMessage that is EOF terminated.

@normanmaurer normanmaurer added the defect label Oct 31, 2016

@normanmaurer normanmaurer added this to the 4.0.43.Final milestone Oct 31, 2016

@normanmaurer normanmaurer self-assigned this Oct 31, 2016

@normanmaurer

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2016

assertFalse(channel.finish());
}

// See See https://github.com/netty/netty/issues/5892

This comment has been minimized.

Copy link
@ldaley

ldaley Oct 31, 2016

Contributor

two “See”'s, if you care.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Oct 31, 2016

Author Member

+1

@ldaley

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2016

Great, thanks. Will this be in 4.1.7? Is there a planned release date for this?

@normanmaurer

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2016

@alkemist yes it will be... I would say end of next week.

@ldaley

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2016

Thanks again.

[#5892] Correct handle HttpMessage that is EOF terminated
Motivation:

We need to ensure we not add the Transfer-Encoding header if the HttpMessage is EOF terminated.

Modifications:

Only add the Transfer-Encoding header if an Content-Length header is present.

Result:

Correctly handle HttpMessage that is EOF terminated.

@normanmaurer normanmaurer force-pushed the eof_content_decoder branch from 24adf07 to 683a302 Oct 31, 2016

@normanmaurer

This comment has been minimized.

Copy link
Member Author

commented Nov 1, 2016

Cherry-picked into 4.1 (8269e0f) and 4.0 (9e8e4d2)

@normanmaurer normanmaurer deleted the eof_content_decoder branch Nov 1, 2016

@jroper

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2016

In Play we use this for decoding gzip/deflate requests, but this wouldn't impact there since request bodies can't be EOF terminated - I guess it only impacts http clients that decompress responses (or use the HttpContentDecoder in some other way). @slandelle may be interested - does async-http-client use the HttpContentDecompressor in its pipeline? And does it ever automatically set content encoding gzip/deflate headers?

@slandelle

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2016

@jroper Yes, AHC uses HttpContentDecompressor and yes, it adds the Accept-Encoding request header by default. It seems there's a hole in our integration tests, thanks for the heads up!

I wasn't able to create a test case for this because HttpContentEncoder enforces chunked responses. I discovered this issue when using Netty to reverse proxy a Jetty server, that uses EOF framing for streamed responses when not using keepalive.

@alkemist I'd be very interested to add the test case you mention in #5892 in our integration test suite as its Jetty based.

@slandelle

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2016

Oh my... So yes, I confirm that I can reproduce the issue with Netty 4.0.42 and that 9e8e4d2 fixes it. Looking forward to the next release :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.