Jetty 12 : precompressed content support for `ResourceService` #8595

joakime · 2022-09-15T21:31:59Z

Reenable test cases for precompressed content.

Fix implementation to support precompressed content.

Protect bad usages of CachingContentFactory

…errides

lorban · 2022-09-19T08:46:05Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/EtagUtil.java

+     * @param suffix the suffix for the ETag
+     * @return the weak etag
+     */
+    public static String calcWeakEtag(Path path, String suffix)


Shouldn't this method accept a Resource instead of a Path?

The weak etag is calculated from the path name + last modified timestamp.

Not all resources have them (eg: MemoryResource has neither).
Can't use a ResourceCollection here either, as the path name and last modified timestamp won't always represent the same path object.
It's better to use Path, as even with ResourceCollection in the mix, we'll get a Path then use it.
This is the same Path that is used by the HttpContent to serve the same content.

BTW, Don't get hung up on MemoryResource, the HttpContentFactory reorg PR coming after this will make the case to remove MemoryResource entirely.

lorban · 2022-09-19T08:46:10Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/EtagUtil.java

+     * @param path the path to calculate from
+     * @return the weak etag
+     */
+    public static String calcWeakEtag(Path path)


Shouldn't this method accept a Resource instead of a Path?

Different method supports Resource now, this method still exists.

lorban · 2022-09-19T08:47:20Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ResourceHttpContent.java

@@ -119,13 +119,13 @@ public HttpField getETag()
    @Override
    public String getETagValue()
    {
-        return _resource.getWeakETag();
+        return EtagUtil.calcWeakEtag(_resource.getPath());


This would only work with path-backed resources. Shouldn't we be capable of calculating the etag of a potential MemoryResource?

Fixed in this PR.

lorban · 2022-09-19T08:47:45Z

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/CachedContentFactory.java

@@ -405,7 +406,7 @@ public class CachedHttpContent implements HttpContent

            _lastAccessed = Instant.now();

-            _etag = CachedContentFactory.this._etags ? new PreEncodedHttpField(HttpHeader.ETAG, resource.getWeakETag()) : null;
+            _etag = CachedContentFactory.this._etags ? new PreEncodedHttpField(HttpHeader.ETAG, EtagUtil.calcWeakEtag(resource.getPath())) : null;


This would only work with path-backed resources. Shouldn't we be capable of calculating the etag of a potential MemoryResource?

There's no ETag support for MemoryResource as it exists currently.

The HttpContentFactory reorg PR (after this one) will remove MemoryResource in favor of a StaticHttpContent object fallback instead (which can hold the etag based on the URI and JAR file last timestamp)

Fixed in this PR anyway.

lorban · 2022-09-19T08:49:33Z

jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ResourceHandlerTest.java

+        assertThat(response, containsHeaderValue(HttpHeader.VARY, "Accept-Encoding"));
+        assertThat(response, containsHeaderValue(HttpHeader.CONTENT_ENCODING, "br"));
+        String body = response.getContent();
+        assertThat(body, containsString("fake br"));


Why only fake br?

The contents don't actually have to be compressed to satisfy the test.
The ResourceService doesn't compress it anyway, it just serves the content as-is from the Resource Base.

But it confuses the reader the fact that you test for fake br only, while at the beginning of the test the whole content is fake brotli.
I would test for the whole content, since it's so small, to avoid such scratching head moment from other readers of this test ("why only fake br"?).

lorban · 2022-09-19T08:50:29Z

jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ResourceHandlerTest.java

@@ -1892,35 +1912,6 @@ public void testDirectoryOfCollections() throws Exception
        assertThat(response.getStatus(), is(HttpStatus.OK_200));
    }

-    @Test
-    public void testEtagIfMatchAlwaysFailsDueToWeakEtag() throws Exception


Why did you remove this test?

It's a bad/duplicate test.

There are other tests that test the same condition.
testIfEtag() for example.
And those expect a 200 OK, not a 412 precondition failure for the same request.

The origin of this test is unknown.
It is not present in jetty-11.0.x.
I can trace as far back as 04acdb7 but not before then.
It seems to have been created during the hackathon.
But etags and preconditions were not working back then.

Interestingly, the github search also shows nothing for this test case...
https://github.com/eclipse/jetty.project/search?q=testEtagIfMatchAlwaysFailsDueToWeakEtag&type=code

lorban · 2022-09-19T08:51:54Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/PrecompressedHttpContent.java

@@ -51,20 +51,19 @@ public HttpField getETag()
    @Override
    public String getETagValue()
    {
-        //return _content.getResource().getWeakETag(_format.getEtagSuffix());
-        return null;
+        return EtagUtil.calcWeakEtag(_content.getResource().getPath(), _format.getEtagSuffix());


This would only work with path-backed resources. Shouldn't we be capable of calculating the etag of a potential MemoryResource?

Fixing etag support

lorban · 2022-09-19T08:52:03Z

jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/CachedContentFactory.java

@@ -403,7 +404,7 @@ public class CachedHttpContent implements HttpContent

            _lastAccessed = Instant.now();

-            _etag = CachedContentFactory.this._etags ? new PreEncodedHttpField(HttpHeader.ETAG, resource.getWeakETag()) : null;
+            _etag = CachedContentFactory.this._etags ? new PreEncodedHttpField(HttpHeader.ETAG, EtagUtil.calcWeakEtag(resource.getPath())) : null;


This would only work with path-backed resources. Shouldn't we be capable of calculating the etag of a potential MemoryResource?

lorban · 2022-09-19T08:52:07Z

jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/CachedContentFactory.java

@@ -603,7 +604,7 @@ public class CachedPrecompressedHttpContent extends PrecompressedHttpContent
            _content = content;
            _precompressedContent = precompressedContent;

-            _etag = (CachedContentFactory.this._etags) ? new PreEncodedHttpField(HttpHeader.ETAG, _content.getResource().getWeakETag(format.getEtagSuffix())) : null;
+            _etag = (CachedContentFactory.this._etags) ? new PreEncodedHttpField(HttpHeader.ETAG, EtagUtil.calcWeakEtag(_content.getResource().getPath(), format.getEtagSuffix())) : null;


This would only work with path-backed resources. Shouldn't we be capable of calculating the etag of a potential MemoryResource?

…precompressed-support-resource-service

sbordet · 2022-09-20T17:06:13Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CachingContentFactory.java

-                FileTime lastModifiedTime = Files.getLastModifiedTime(_delegate.getResource().getPath());
-                if (lastModifiedTime.equals(_lastModifiedValue))
+                if (!_delegate.getResource().exists())
+                    return false;


Aren't these redundant checks now that the constructor checks them?

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CompressedContentFormat.java

sbordet · 2022-09-20T17:09:58Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/EtagUtil.java

+/**
+ * Utility classes for Etag behaviors
+ */
+public class EtagUtil


Contains more than one utility, so I'd prefer EtagUtils.

Also, add a private empty constructor so it cannot be instantiated.

sbordet · 2022-09-20T17:14:25Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/EtagUtil.java

+     * @param suffix the suffix for the ETag
+     * @return the weak etag
+     */
+    public static String calcWeakEtag(Path path, String suffix)


Prefix calc implies calculations, which to me implies math operations only.
I think something like compute or resolve instead of calc is more appropriate.

sbordet · 2022-09-20T17:18:24Z

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceContentFactory.java

+                    continue; // skip, compressed resource modified before actual resource
+                if (compressedResource.length() >= resource.length())
+                    continue; // skip, not smaller than actual resource
+                // add found precompressed format


Remove the comments, they are just describing what the code does, and the code is very clear.
Less chances that the comments rot.

sbordet · 2022-09-20T17:23:35Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/EtagUtil.java

+        if (suffix != null)
+            b.append(suffix);
+        b.append('"');
+        return b.toString();


Is there a reference for this algorithm? Is it defined by some spec? If so, please add a reference to it.

The Etag is just a string.
The bits and pieces we use are just our take on how to create that string.
I'll add the link to the RFC9110 section in the top level javadoc.

sbordet · 2022-09-20T17:25:07Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/EtagUtil.java

+     * @param suffix the suffix for the ETag
+     * @return the weak etag
+     */
+    public static String calcWeakEtag(Path path, String suffix)


The original implementation in Resource was not using Path at all, so I have the same concerns: this method should take a Resource, not a Path.

The Resource version didn't handle non-path/file-system based Resources properly.

Btw, there is a Resource method in this class for working with Resource to HttpField.

There's also the problem when working with a ResourceCollection there is a difference in .getPath() and .getLastModified() which doesn't match the Resource actually being served in the HttpContent

sbordet · 2022-09-20T17:28:02Z

jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ResourceHandlerTest.java

+        assertThat(response, containsHeaderValue(HttpHeader.VARY, "Accept-Encoding"));
+        assertThat(response, containsHeaderValue(HttpHeader.CONTENT_ENCODING, "br"));
+        String body = response.getContent();
+        assertThat(body, containsString("fake br"));


But it confuses the reader the fact that you test for fake br only, while at the beginning of the test the whole content is fake brotli.
I would test for the whole content, since it's so small, to avoid such scratching head moment from other readers of this test ("why only fake br"?).

sbordet · 2022-09-20T17:32:34Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/EtagUtil.java

+     * @return True if the tags are equal.
+     * @see <a href="https://www.rfc-editor.org/rfc/rfc9110#section-8.8.3.2">RFC9110: Section 8.8.3.2 - Etag Comparison</a>.
+     */
+    public static boolean match(String etag, String etagWithOptionalSuffix)


I would call this method matches because it returns a boolean.
It's kind of a query/question rather than a command.

Signed-off-by: Joakim Erdfelt <[email protected]>

lorban · 2022-09-21T07:23:20Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CachingContentFactory.java

                {
-                    _lastAccessed = NanoTime.now();
-                    return true;
+                    FileTime lastModifiedTime = Files.getLastModifiedTime(_delegate.getResource().getPath());


Since this makes caching only work with path-backed resources, shouldn't there be a check in the constructor to prevent accepting those?

Good point, let me fix this code block.

+ Using Resource.lastModified were possible + CachingHttpContent has better Resource protection, and uses Etag from delegate + CachingHttpContent uses delegate where possible. + Store Etag HttpField in CachingHttpContent and allow override in constructor

joakime added 2 commits September 15, 2022 10:53

Remove Resource.getWeakETag in favor of new EtagUtil

dd9110a

Protect bad usages of CachingContentFactory

Working precompressed content for ResourceService

66c2b34

joakime added the jetty 12 label Sep 15, 2022

joakime requested review from sbordet and lorban September 15, 2022 21:32

joakime self-assigned this Sep 15, 2022

joakime added 3 commits September 16, 2022 09:03

Fixing DefaultServlet handling of ResourceService.writeHttpError() ov…

10c8c99

…errides

Protect NPE in CachingContentFactory.isValid()

b9c0631

Complete callback in DefaultServlet.writeHttpError

4d96c97

joakime marked this pull request as ready for review September 17, 2022 14:39

lorban requested changes Sep 19, 2022

View reviewed changes

joakime added 2 commits September 20, 2022 09:36

Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/jetty-12-…

395df07

…precompressed-support-resource-service

Addressing review comments

b90fa2e

joakime requested a review from lorban September 20, 2022 15:44

sbordet requested changes Sep 20, 2022

View reviewed changes

joakime added 4 commits September 20, 2022 12:52

Testing for whole content

82d681f

Rename EtagUtil to EtagUtils

2f07cf2

Improve MemoryResource handling of name/filename

bcce3ee

EtagUtils handling of Resource

210fe98

joakime requested a review from sbordet September 20, 2022 18:28

Checkstyle and compile fixes

bf5a8c2

Signed-off-by: Joakim Erdfelt <[email protected]>

lorban requested changes Sep 21, 2022

View reviewed changes

joakime added 2 commits September 21, 2022 06:55

Better protection of bad Resource impls in EtagUtils

476bb06

Update CachingContentFactory

a112258

+ Using Resource.lastModified were possible + CachingHttpContent has better Resource protection, and uses Etag from delegate + CachingHttpContent uses delegate where possible. + Store Etag HttpField in CachingHttpContent and allow override in constructor

joakime requested a review from lorban September 21, 2022 11:57

Simpler CachingHttpContent.isValid()

6a9e73f

lorban approved these changes Sep 21, 2022

View reviewed changes

Better handling of PrecompressedHttpContent

de2c101

sbordet approved these changes Sep 21, 2022

View reviewed changes

joakime merged commit 4fe414a into jetty-12.0.x Sep 21, 2022

joakime deleted the fix/jetty-12-precompressed-support-resource-service branch September 21, 2022 14:39

olamy mentioned this pull request Dec 26, 2022

keep matching exception to found errorpage #9090

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jetty 12 : precompressed content support for `ResourceService` #8595

Jetty 12 : precompressed content support for `ResourceService` #8595

joakime commented Sep 15, 2022

lorban Sep 19, 2022

joakime Sep 20, 2022 •

edited

Loading

lorban Sep 19, 2022

joakime Sep 20, 2022

lorban Sep 19, 2022

joakime Sep 20, 2022

lorban Sep 19, 2022

joakime Sep 20, 2022

joakime Sep 20, 2022

lorban Sep 19, 2022

joakime Sep 20, 2022

sbordet Sep 20, 2022

lorban Sep 19, 2022

joakime Sep 20, 2022

lorban Sep 19, 2022

joakime Sep 20, 2022

lorban Sep 19, 2022

joakime Sep 20, 2022

lorban Sep 19, 2022

joakime Sep 20, 2022

sbordet Sep 20, 2022

joakime Sep 21, 2022

sbordet Sep 20, 2022

sbordet Sep 20, 2022

sbordet Sep 20, 2022

sbordet Sep 20, 2022

joakime Sep 20, 2022

sbordet Sep 20, 2022

joakime Sep 20, 2022

joakime Sep 20, 2022

sbordet Sep 20, 2022

sbordet Sep 20, 2022

lorban Sep 21, 2022

joakime Sep 21, 2022

Jetty 12 : precompressed content support for ResourceService #8595

Jetty 12 : precompressed content support for ResourceService #8595

Conversation

joakime commented Sep 15, 2022

Choose a reason for hiding this comment

joakime Sep 20, 2022 • edited Loading

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

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

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

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

Choose a reason for hiding this comment

Jetty 12 : precompressed content support for `ResourceService` #8595

Jetty 12 : precompressed content support for `ResourceService` #8595

joakime Sep 20, 2022 •

edited

Loading