Fix/7.x/compression dotnetcore #3921
Merged
Conversation
This file contains hidden or 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
This commit removes adding Accept-Encoding headers again, when constructing the requestMessage. These headers are added by the default HttpClientHandler implementation, with AutomaticDecompression. There is the possibility that a user overrides CreateHttpClientHandler() and does not set AutomaticDecompression, but does not override CreateRequestMessage(), and is relying on CreateHttpRequestMessage to add the Accept-Encoding headers when using HttpCompression. This commit will break that usage pattern, but I think this is acceptable to do, because we should optimize for the default behaviour i.e. we _could_ check that Accept-Encoding headers have been set when HttpCompression is enabled, inside CreateHttpRequestMessage(), and set them if not, but this is additional overhead to perform on every request. Instead, a user overriding CreateHttpClientHandler() and not setting AutomaticDecompression, and not overriding CreateRequestMessage(), when wishing to use HttpCompression, should set AutomaticDecompression inside their overridden implementation.
russcam
approved these changes
Jul 5, 2019
Contributor
russcam
left a comment
russcam
left a comment
There was a problem hiding this comment.
LGTM. I pushed a small fix for setting Accept-Encoding headers twice
| //.EnableHttpCompression() | ||
| .EnableHttpCompression(TestConfiguration.Instance.Random.HttpCompression) | ||
| #if DEBUG | ||
| .EnableDebugMode() |
Contributor
There was a problem hiding this comment.
Not something to address in this PR, but this #if preprocessor directive to enable debug mode looks superfluous, because it's enabled already on line 45.
russcam
pushed a commit
that referenced
this pull request
Jul 5, 2019
) * GzipStream was wrapping completed task handler not the writable stream * Make sure HttpCompression is enabled randomly when running the integration tests * Remove adding Accept-Encoding twice This commit removes adding Accept-Encoding headers again, when constructing the requestMessage. These headers are added by the default HttpClientHandler implementation, with AutomaticDecompression. There is the possibility that a user overrides CreateHttpClientHandler() and does not set AutomaticDecompression, but does not override CreateRequestMessage(), and is relying on CreateHttpRequestMessage to add the Accept-Encoding headers when using HttpCompression. This commit will break that usage pattern, but I think this is acceptable to do, because we should optimize for the default behaviour i.e. we _could_ check that Accept-Encoding headers have been set when HttpCompression is enabled, inside CreateHttpRequestMessage(), and set them if not, but this is additional overhead to perform on every request. Instead, a user overriding CreateHttpClientHandler() and not setting AutomaticDecompression, and not overriding CreateRequestMessage(), when wishing to use HttpCompression, should set AutomaticDecompression inside their overridden implementation. Fixes #3913
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.
On .NET with our move to using non buffered writes to the stream I moved the
GzipStreamto wrap the stream passed toCompleteTaskOnCloseStreamwhich was inadvertently breaking the stream becomming ready for writing resulting in no data being sent over the wire.This PR also randomly enables
HttpCompressiononConnectionSettingsbased on the test seed. Something I could have sworn was already in place but found only aTODO😿