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.
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
Fixes #8926 - HttpClient GZIPContentDecoder should remove Content-Len… #10326
Fixes #8926 - HttpClient GZIPContentDecoder should remove Content-Len… #10326
Changes from all commits
a54527e
eed61fa
e0e304e
e54187c
6c90696
d9f9de7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a header can have multiple CONTENT_ENCODING fields which mean the same as a single comma separated field. But it is 1am and it could be space separate fields I'm thinking of?
So probably best to use
HttpFields.getCSV
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we wanted to be really nasty, we should strip "identity" encodings off the end.... but the server doesn't do that (anymore)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the times
Content-Encoding
is single-valued.As you seem concerned about hot paths, I chose not to use
getCSV()
because in the single-value case allocates aList
and aQuotedCSV
for nothing, while the code above only does so for multi-values.Doing a first
get()
and thengetCSV()
iterates over the headers twice, although should be a small list.What do you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a vexed question and now I'm concerned the server doesn't well handle the case of multiple
HttpField
s.If we are struggling to come up with something that is both simple, clear and efficient, then probably the
HttpFields
API is not good enough.Perhaps we need a method to ask
HttpFields
if it has a value as the last entry in a list of values, with the option of removing that entry. This could be implemented efficiently without having to tokenize the list twice. I'd be happy with an implementation that worked well for single valued lists, but was more expensive for multi valued lists.Something like:
and on mutable:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbordet I've created #10340 that might be useful hear as an efficient way to find the last value. I've not done anything about removing the last value until I get feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbordet I've further update #10340 with a listIterator that make operating on the last item simpler. So now in the server GzipRequest, I just copy the
HttpFields
to a mutable, then iterate in reverse over it correcting the headers found.This will have merge hell with #10339 and #10308