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

Adjust Content-Length header when encoding Full Responses #6809

Closed

Conversation

bryce-anderson
Copy link
Contributor

Motivation:
If a full HttpResponse with a Content-Length header is encoded by the HttpContentEncoder subtypes the Content-Length header is removed and the message is set to Transfer-Encoder: chunked. This is an unnecessary loss of information about the message content.

Modifications:

  • If a full HttpResponse has a Content-Length header, the header is adjusted after encoding instead of converting the message to Chunked-Transfer encoding.

Result:
Fixes #6808.

@@ -205,6 +208,25 @@ protected void encode(ChannelHandlerContext ctx, HttpObject msg, List<Object> ou
}
}

private void encodeFullResponse(HttpResponse newRes, HttpContent content, List<Object> out) {
int existingMessages = out.size();
assert encodeContent(content, out);
Copy link
Contributor

Choose a reason for hiding this comment

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

in runtime an assert block is ignoring if -ea option has not specified.

@@ -205,6 +208,25 @@ protected void encode(ChannelHandlerContext ctx, HttpObject msg, List<Object> ou
}
}

private void encodeFullResponse(HttpResponse newRes, HttpContent content, List<Object> out) {
int existingMessages = out.size();
assert encodeContent(content, out);
Copy link
Member

Choose a reason for hiding this comment

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

We don't want this to be an assert, right? assert statements can be disabled via command line and optimized away by JIT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

fullRes.headers().set(HttpHeaderNames.CONTENT_LENGTH, fullRes.content().readableBytes());
ch.writeOutbound(fullRes);

Object o = ch.readOutbound();
Copy link
Member

Choose a reason for hiding this comment

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

consider just directly casting to the object you expect like the other tests ... a class cast exception will be thrown if the expected type is not read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@Scottmitch Scottmitch left a comment

Choose a reason for hiding this comment

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

lgtm ... @normanmaurer - wdyt?

@normanmaurer normanmaurer self-assigned this Jun 6, 2017
@normanmaurer
Copy link
Member

@Scottmitch yeah LGTM... @bryce-anderson can you please squash ? After this I will pull in

Motivation:
If a full HttpResponse with a Content-Length header is encoded by the HttpContentEncoder subtypes the Content-Length header is removed and the message is set to Transfer-Encoder: chunked. This is an unnecessary loss of information about the message content.

Modifications:
- If a full HttpResponse has a Content-Length header, the header is adjusted after encoding.

Result:
Complete messages continue to have the Content-Length header after encoding.
@bryce-anderson
Copy link
Contributor Author

Squished. Thanks for the reviews, @fenik17, @Scottmitch and @normanmaurer.

@normanmaurer
Copy link
Member

Cherry-picked into 4.0 (7297966) and 4.1 (9fa3e55)

@normanmaurer normanmaurer added this to the 4.0.48.Final milestone Jun 6, 2017
@bryce-anderson
Copy link
Contributor Author

@normanmaurer, @Scottmitch I just occurred to me: do any of the content encoders emit a raw ByteBuf? If so, this PR will break those cases since they won't be included in the adjusted content-length header. It looks like the HttpObjectEncoder is happy to encode a ByteBuf, so I think I may have just introduced a bug, which I'm happy to fix.

@bryce-anderson
Copy link
Contributor Author

Okay, looks like my worries were unfounded: all content emitted from the compressors is wrapped in DefaultHttpContent in the fetchEncoderOutput method. Sorry for the noise.

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

Successfully merging this pull request may close these issues.

None yet

4 participants