Skip to content
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.

Sign up for GitHub

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

Fix Caching ContentFactories in Jetty-12 #8621

Merged
merged 9 commits into from
Oct 4, 2022
Merged

Fix Caching ContentFactories in Jetty-12 #8621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c6305ff
Improvements to CachingContentFactory for Jetty 12
lachlan-roberts Sep 26, 2022
bed034c
more fixes for test cases in DefaultServletTest
lachlan-roberts Sep 27, 2022
c5b4dde
Fix ETag header in 304 responses for ResourceService, provide precomp…
lachlan-roberts Sep 27, 2022
b503d93
fix some more tests in DefaultServletTest for EE10
lachlan-roberts Sep 27, 2022
c58f769
refactor test for resource lastModified comparisons
lachlan-roberts Sep 27, 2022
ae071c5
release precompressed content buffer in PrecompressedHttpContent & us…
lachlan-roberts Sep 27, 2022
1508eb6
keep the ETag header in ServletContextResponse.resetContent if Respon…
lachlan-roberts Sep 28, 2022
98f9dee
remove parent from CachingContentFactory
lachlan-roberts Sep 29, 2022
1c9ac9c
Update javadoc for CachingContentFactory.isCacheable()
lachlan-roberts Sep 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 28 additions & 2 deletions jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CachingContentFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,32 @@ public void flushCache()
}
}

/**
* Tests whether the given HttpContent is cacheable, and if there is enough room to fit it in the cache.
*
* @param httpContent the HttpContent to test.
* @return whether the HttpContent is cacheable.
*/
protected boolean isCacheable(HttpContent httpContent)
{
if (httpContent == null)
return false;

if (httpContent.getResource().isDirectory())
return false;

if (_maxCachedFiles <= 0)
return false;

// Will it fit in the cache?
long len = httpContent.getContentLengthValue();
if (len <= 0)
return false;
if (isUseFileMappedBuffer())
return true;
return ((len <= _maxCachedFileSize) && (len + getCachedSize() <= _maxCacheSize));
}

@Override
public HttpContent getContent(String path) throws IOException
{
Expand All @@ -178,14 +204,14 @@ public HttpContent getContent(String path) throws IOException
}

HttpContent httpContent = _authority.getContent(path);
// Do not cache directories or files that are too big
if (httpContent != null && !httpContent.getResource().isDirectory() && httpContent.getContentLengthValue() <= _maxCachedFileSize)
if (isCacheable(httpContent))
{
httpContent = cachingHttpContent = new CachingHttpContent(path, httpContent);
_cache.put(path, cachingHttpContent);
_cachedSize.addAndGet(cachingHttpContent.calculateSize());
shrinkCache();
}

return httpContent;
}

Expand Down
4 changes: 2 additions & 2 deletions jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/PrecompressedHttpContent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,12 +139,12 @@ public Map<CompressedContentFormat, HttpContent> getPrecompressedContents()
@Override
public ByteBuffer getBuffer()
{
return _content.getBuffer();
return _precompressedContent.getBuffer();
}

@Override
public void release()
{
_content.release();
_precompressedContent.release();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it not release both _content and _precompressedContent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
Loading