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

HttpObjectAggregator only set Content-Length is not already set. #3291

Closed
wants to merge 1 commit into from

Conversation

beckje01
Copy link
Contributor

Motivation:
HEAD requests will have a Content-Length set that doesn't match the
actual length. So we only want to set Content-Length header if it isn't
already set.

Modifications:
If check around setting the Content-Length

Result:
A HEAD request will no correctly return the specified Content-Length
instead of the body length

Motivation:

HEAD requests will have a Content-Length set that doesn't match the
actual length. So we only want to set Content-Length header if it isn't
already set.

Modifications:

If check around setting the Content-Length
Result:

A HEAD request will no correctly return the specified Content-Length
instead of the body length
@netkins
Copy link

netkins commented Dec 24, 2014

AUTOMATED MESSAGE for ADMINS:
Please verify and accept the pull request.
To accept, say @netkins accept
To build again, say @netkins build
To whitelist the author, say @netkins whitelist

@normanmaurer
Copy link
Member

@beckje01 you are right.. please adjust to only do this for HEAD requests....

From RFC:

The Content-Length entity-header field indicates the size of the entity-body, in decimal number of OCTETs, sent to the recipient or, in the case of the HEAD method, the size of the entity-body that would have been sent had the request been a GET.

@normanmaurer
Copy link
Member

@netkins accept

@beckje01
Copy link
Contributor Author

@normanmaurer I'm not sure how I can tell with just the FullHttpMessage what type of request was made to start. In most cases if a content-length is set it should be passed through unless it was chunked in which case the content-length header shouldn't be set at all.

I could specifically check if the content-length header is > 0 and the content size of the message is 0 and for that case not override the existing content-length header; that should cover the HEAD scenario correctly. Does that sound like the best option?

@beckje01
Copy link
Contributor Author

I think I made this very confusing due to how I worded the first part of this, let my try rephrasing.

A HEAD request is made to some server and the response is sent to the HttpObjectAggregator in which case we just have a AggregatedFullHttpResponse which doesn't have the method available. The finishAggregation is called and even if the Content-Length header was set by the responding server the value is recalculated, this recalculation is probably a good thing most of the time and in the case of chunked encoding it is necessary but in the special case of HEAD we don't want to override what the server sent us.

@normanmaurer
Copy link
Member

@beckje01 ah ok got it... yeah you are right your fix should be ok then! :)

@normanmaurer normanmaurer added this to the 4.0.25.Final milestone Dec 25, 2014
@beckje01
Copy link
Contributor Author

Any idea what that failure is on the build?

@normanmaurer
Copy link
Member

@beckje01 need to investigate... not related to your change

@normanmaurer normanmaurer self-assigned this Dec 25, 2014
@normanmaurer
Copy link
Member

@beckje01 thanks... cherry-picked into 4.0 (0dca08a), 4.1 (5ba4fdf) and master (20f5974)

@beckje01
Copy link
Contributor Author

@normanmaurer No thank you!

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.

None yet

3 participants