Fixes #8926 - HttpClient GZIPContentDecoder should remove Content-Len… #10326

sbordet · 2023-08-16T16:57:20Z

…gth and Content-Encoding: gzip

Now Content-Length and Content-Encoding are removed/modified by the decoder. In this way, applications have a correct sets of headers to decide whether to decode the content themselves.

…gth and Content-Encoding: gzip Now Content-Length and Content-Encoding are removed/modified by the decoder. In this way, applications have a correct sets of headers to decide whether to decode the content themselves. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

joakime · 2023-08-16T17:45:49Z

This broke the org.eclipse.jetty.http2.client.http.ContentLengthTest.testGzippedContentLengthAddedByServer() tests

jetty-client/src/main/java/org/eclipse/jetty/client/ContentDecoder.java

jetty-client/src/main/java/org/eclipse/jetty/client/GZIPContentDecoder.java

lorban · 2023-08-17T09:40:55Z

jetty-client/src/main/java/org/eclipse/jetty/client/HttpReceiver.java

+        {
+            // Pick the last content encoding from the server.
+            String contentEncoding = contentEncodings.get(contentEncodings.size() - 1);
+            decoder = getHttpDestination().getHttpClient().getContentDecoderFactories().stream()


As @gregw would say: don't use streams on the fast path.

jetty-client/src/main/java/org/eclipse/jetty/client/HttpReceiver.java

lorban · 2023-08-17T09:47:01Z

jetty-client/src/main/java/org/eclipse/jetty/client/HttpResponse.java

@@ -87,6 +87,11 @@ public HttpFields getHeaders()
        return headers.asImmutable();
    }

+    protected HttpFields.Mutable headers()


There is already the public HttpResponse headers(Consumer<HttpFields.Mutable> consumer) API that allows mutating the headers. IMHO one of these two methods should go.

jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientGZIPTest.java

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

jetty-client/src/main/java/org/eclipse/jetty/client/GZIPContentDecoder.java

jetty-client/src/main/java/org/eclipse/jetty/client/HttpReceiver.java

gregw · 2023-08-17T14:50:48Z

jetty-client/src/main/java/org/eclipse/jetty/client/HttpReceiver.java

+            String contentEncoding = responseHeaders.get(HttpHeader.CONTENT_ENCODING);
+            if (contentEncoding != null)
            {
+                int comma = contentEncoding.indexOf(",");
+                if (comma > 0)
+                {
+                    QuotedCSV parser = new QuotedCSV(false);
+                    parser.addValue(contentEncoding);
+                    List<String> values = parser.getValues();
+                    contentEncoding = values.get(values.size() - 1);
+                }


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

If we wanted to be really nasty, we should strip "identity" encodings off the end.... but the server doesn't do that (anymore)

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 a List and a QuotedCSV for nothing, while the code above only does so for multi-values.

Doing a first get() and then getCSV() iterates over the headers twice, although should be a small list.

What do you prefer?

It is a vexed question and now I'm concerned the server doesn't well handle the case of multiple HttpFields.

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:

String contentEncoding = responseHeaders.getLastCSV(HttpHeader.CONTENT_ENCODING);

and on mutable:

String contentEncoding = responseHeaders.takeLastCSV(HttpHeader.CONTENT_ENCODING);

@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.

@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

gregw · 2023-08-18T07:58:26Z

I'll fix up the efficiency and correctness if content-encoding handling in #10340 to be merged separately.

lorban

Just one tiny nit, otherwise LGTM.

lorban · 2023-08-18T10:56:21Z

jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientGZIPTest.java


        ByteArrayOutputStream output = new ByteArrayOutputStream();
        try (InputStream input = listener.getInputStream())
        {
            IO.copy(input, output);
        }
        assertArrayEquals(content, output.toByteArray());
+        // After the content has been coded, the length is known again.


After the content has been encoded?

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet requested review from lorban and joakime August 16, 2023 16:57

sbordet linked an issue Aug 16, 2023 that may be closed by this pull request

HttpClient GZIPContentDecoder should remove Content-Length and Content-Encoding: gzip #8926

Closed

sbordet requested a review from gregw August 16, 2023 21:33

gregw requested changes Aug 16, 2023

View reviewed changes

lorban requested changes Aug 17, 2023

View reviewed changes

sbordet added 2 commits August 17, 2023 12:03

Updates after review.

eed61fa

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Updates after review.

e0e304e

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet requested review from gregw and lorban August 17, 2023 10:25

sbordet added 2 commits August 17, 2023 13:49

Fixed test.

e54187c

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Fixed test.

6c90696

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

gregw requested changes Aug 17, 2023

View reviewed changes

jetty-client/src/main/java/org/eclipse/jetty/client/GZIPContentDecoder.java Outdated Show resolved Hide resolved

jetty-client/src/main/java/org/eclipse/jetty/client/HttpReceiver.java Outdated Show resolved Hide resolved

gregw requested changes Aug 17, 2023

View reviewed changes

gregw mentioned this pull request Aug 18, 2023

Implement containsLast in HttpFields #10340

Merged

gregw previously approved these changes Aug 18, 2023

View reviewed changes

lorban previously approved these changes Aug 18, 2023

View reviewed changes

Fixed typo from review.

d9f9de7

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet dismissed stale reviews from lorban and gregw via d9f9de7 August 21, 2023 06:57

sbordet merged commit e91a689 into jetty-10.0.x Aug 21, 2023
2 of 3 checks passed

sbordet deleted the fix/jetty-10-8926-httpclient-content-encoding branch August 21, 2023 06:58

sbordet added a commit that referenced this pull request Aug 21, 2023

Removed break statement wrongly introduced by #10326.

36faf1d

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #8926 - HttpClient GZIPContentDecoder should remove Content-Len… #10326

Fixes #8926 - HttpClient GZIPContentDecoder should remove Content-Len… #10326

sbordet commented Aug 16, 2023

joakime commented Aug 16, 2023

lorban Aug 17, 2023

lorban Aug 17, 2023

gregw Aug 17, 2023

gregw Aug 17, 2023

sbordet Aug 17, 2023

gregw Aug 17, 2023

gregw Aug 18, 2023

gregw Aug 18, 2023

gregw commented Aug 18, 2023

lorban left a comment

lorban Aug 18, 2023

Fixes #8926 - HttpClient GZIPContentDecoder should remove Content-Len… #10326

Fixes #8926 - HttpClient GZIPContentDecoder should remove Content-Len… #10326

Conversation

sbordet commented Aug 16, 2023

joakime commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregw commented Aug 18, 2023

lorban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment