Fix Caching ContentFactories in Jetty-12 #8621
Merged
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
Signed-off-by: Lachlan Roberts <[email protected]>
Signed-off-by: Lachlan Roberts <[email protected]>
…ressedFormats to DefaultServlet Signed-off-by: Lachlan Roberts <[email protected]>
Signed-off-by: Lachlan Roberts <[email protected]>
Signed-off-by: Lachlan Roberts <[email protected]>
…e HttpContent.ContentFactory for parent in CachingContentFactory Signed-off-by: Lachlan Roberts <[email protected]>
joakime
previously requested changes
Sep 27, 2022
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java
Outdated
Show resolved
Hide resolved
joakime
reviewed
Sep 27, 2022
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java
Outdated
Show resolved
Hide resolved
…se is 304 Signed-off-by: Lachlan Roberts <[email protected]>
Signed-off-by: Lachlan Roberts <[email protected]>
sbordet
reviewed
Sep 29, 2022
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CachingContentFactory.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public void release() | ||
| { | ||
| _content.release(); | ||
| _precompressedContent.release(); |
There was a problem hiding this comment.
Should it not release both _content and _precompressedContent?
There was a problem hiding this comment.
I don't know. The _content will be accessible separately to the pre-compressed version and will have separate entry in the cache, so maybe this would result in a double release?
At the moment it looks there are no implementations of HttpContent.release() which actually do anything, so this currently has no effect.
| { | ||
| return !resource2.lastModified().isAfter(resource1.lastModified()); | ||
| } | ||
|
|
There was a problem hiding this comment.
I found the original version more readable, but ok.
Rename to isNewerThanOrEqual().
There was a problem hiding this comment.
The original version was wrong and was causing many test failures.
sbordet
approved these changes
Sep 29, 2022
Signed-off-by: Lachlan Roberts <[email protected]>
lorban
approved these changes
Sep 30, 2022
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.
This PR removes the old
CachedContentFactorywhich was only used in tests, and brings over some of its functionality to theCachingContentFactory.Fixes the
CachingContentFactoryto make it pass tests, and re-enable the tests.