Fix Content-Length header removal after content compression #2863
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation: The ContentEncodingHttpServiceFilter is meant to remove the `Content-Length` header if it does compression, because any such header from earlier stages will be incorrect after compression. An incorrect `Content-Length` header will then cause down-stream clients to be confused and potentially stall, timeout, or produce an unexpected close error, when they don't receive their promised number of bytes (most likely fewer than promised, due to compression). Modifications: The ContentEncodingHttpServiceFilter incorrectly removed the `Content-Length` header from the _request_ object, and this has been fixed so it now correctly removes the header from the _response_ object. There was a test for this behavior, but the test wasn't actually hitting this code because it used a client that did not advertise support for compressed responses. The test has also been fixed, adding the case where the client supports compressed responses. We now test both that the `Content-Length` header is removed when the response is compressed, and that the header stays when the response is not compressed. Result: This fixes the issue showcased by apple#2862
dramaticlly
approved these changes
Mar 6, 2024
Scottmitch
approved these changes
Mar 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
The ContentEncodingHttpServiceFilter is meant to remove the
Content-Lengthheader if it does compression, because any such header from earlier stages will be incorrect after compression. An incorrectContent-Lengthheader will then cause down-stream clients to be confused and potentially stall, timeout, or produce an unexpected close error, when they don't receive their promised number of bytes (most likely fewer than promised, due to compression).Modifications:
The ContentEncodingHttpServiceFilter incorrectly removed the
Content-Lengthheader from the request object, and this has been fixed so it now correctly removes the header from the response object. There was a test for this behavior, but the test wasn't actually hitting this code because it used a client that did not advertise support for compressed responses. The test has also been fixed, adding the case where the client supports compressed responses. We now test both that theContent-Lengthheader is removed when the response is compressed, and that the header stays when the response is not compressed.Result:
This fixes the issue showcased by #2862