Skip to content

Commit 057ba3e

Browse files
lorbanlorban
committed
#12681 atomically retain the buffer with a check that it still is in the cache, otherwise retain fails + remove single-threaded shrinking
Signed-off-by: Ludovic Orban <[email protected]>
1 parent 73e6058 commit 057ba3e

File tree

2 files changed

+130
-39
lines changed

2 files changed

+130
-39
lines changed

‎jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/content/CachingHttpContentFactory.java

+31-39
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,6 @@ public class CachingHttpContentFactory implements HttpContent.Factory
6868
private final HttpContent.Factory _authority;
6969
private final ConcurrentHashMap<String, CachingHttpContent> _cache = new ConcurrentHashMap<>();
7070
private final AtomicLong _cachedSize = new AtomicLong();
71-
private final AtomicBoolean _shrinking = new AtomicBoolean();
7271
private final ByteBufferPool _bufferPool;
7372
private int _maxCachedFileSize = DEFAULT_MAX_CACHED_FILE_SIZE;
7473
private int _maxCachedFiles = DEFAULT_MAX_CACHED_FILES;
@@ -149,46 +148,35 @@ public void setUseDirectByteBuffers(boolean useDirectByteBuffers)
149148

150149
private void shrinkCache()
151150
{
152-
// Only 1 thread shrinking at once
153-
if (_shrinking.compareAndSet(false, true))
151+
// While we need to shrink
152+
int numCacheEntries = _cache.size();
153+
while (numCacheEntries > 0 && (numCacheEntries > _maxCachedFiles || _cachedSize.get() > _maxCacheSize))
154154
{
155-
try
155+
// Scan the entire cache and generate an ordered list by last accessed time.
156+
SortedSet<CachingHttpContent> sorted = new TreeSet<>((c1, c2) ->
156157
{
157-
// While we need to shrink
158-
int numCacheEntries = _cache.size();
159-
while (numCacheEntries > 0 && (numCacheEntries > _maxCachedFiles || _cachedSize.get() > _maxCacheSize))
160-
{
161-
// Scan the entire cache and generate an ordered list by last accessed time.
162-
SortedSet<CachingHttpContent> sorted = new TreeSet<>((c1, c2) ->
163-
{
164-
long delta = NanoTime.elapsed(c2.getLastAccessedNanos(), c1.getLastAccessedNanos());
165-
if (delta != 0)
166-
return delta < 0 ? -1 : 1;
167-
168-
delta = c1.getContentLengthValue() - c2.getContentLengthValue();
169-
if (delta != 0)
170-
return delta < 0 ? -1 : 1;
171-
172-
return c1.getKey().compareTo(c2.getKey());
173-
});
174-
sorted.addAll(_cache.values());
175-
176-
// TODO: Can we remove the buffers from the content before evicting.
177-
// Invalidate least recently used first
178-
for (CachingHttpContent content : sorted)
179-
{
180-
if (_cache.size() <= _maxCachedFiles && _cachedSize.get() <= _maxCacheSize)
181-
break;
182-
removeFromCache(content);
183-
}
184-
185-
numCacheEntries = _cache.size();
186-
}
187-
}
188-
finally
158+
long delta = NanoTime.elapsed(c2.getLastAccessedNanos(), c1.getLastAccessedNanos());
159+
if (delta != 0)
160+
return delta < 0 ? -1 : 1;
161+
162+
delta = c1.getContentLengthValue() - c2.getContentLengthValue();
163+
if (delta != 0)
164+
return delta < 0 ? -1 : 1;
165+
166+
return c1.getKey().compareTo(c2.getKey());
167+
});
168+
sorted.addAll(_cache.values());
169+
170+
// TODO: Can we remove the buffers from the content before evicting.
171+
// Invalidate least recently used first
172+
for (CachingHttpContent content : sorted)
189173
{
190-
_shrinking.set(false);
174+
if (_cache.size() <= _maxCachedFiles && _cachedSize.get() <= _maxCacheSize)
175+
break;
176+
removeFromCache(content);
191177
}
178+
179+
numCacheEntries = _cache.size();
192180
}
193181
}
194182

@@ -253,7 +241,6 @@ public HttpContent getContent(String path) throws IOException
253241
if (!isCacheable(httpContent))
254242
return httpContent;
255243

256-
// The re-mapping function may be run multiple times by compute.
257244
AtomicBoolean added = new AtomicBoolean();
258245
cachingHttpContent = _cache.computeIfAbsent(path, key ->
259246
{
@@ -418,7 +405,12 @@ public String getKey()
418405
@Override
419406
public boolean retain()
420407
{
421-
return _referenceCount.tryRetain();
408+
// Retain only if the content is still in the cache.
409+
return _cache.computeIfPresent(_cacheKey, (s, cachingHttpContent) ->
410+
{
411+
_referenceCount.retain();
412+
return cachingHttpContent;
413+
}) != null;
422414
}
423415

424416
@Override

‎jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/content/CachingHttpContentFactoryTest.java

+99
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
//
2+
// ========================================================================
3+
// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others.
4+
//
5+
// This program and the accompanying materials are made available under the
6+
// terms of the Eclipse Public License v. 2.0 which is available at
7+
// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
8+
// which is available at https://www.apache.org/licenses/LICENSE-2.0.
9+
//
10+
// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
11+
// ========================================================================
12+
//
13+
14+
package org.eclipse.jetty.http.content;
15+
16+
import java.io.ByteArrayOutputStream;
17+
import java.nio.charset.StandardCharsets;
18+
import java.nio.file.Files;
19+
import java.nio.file.Path;
20+
21+
import org.eclipse.jetty.http.MimeTypes;
22+
import org.eclipse.jetty.io.ArrayByteBufferPool;
23+
import org.eclipse.jetty.io.ByteBufferPool;
24+
import org.eclipse.jetty.toolchain.test.jupiter.WorkDir;
25+
import org.eclipse.jetty.toolchain.test.jupiter.WorkDirExtension;
26+
import org.eclipse.jetty.util.BufferUtil;
27+
import org.eclipse.jetty.util.resource.ResourceFactory;
28+
import org.junit.jupiter.api.AfterEach;
29+
import org.junit.jupiter.api.BeforeEach;
30+
import org.junit.jupiter.api.Test;
31+
import org.junit.jupiter.api.extension.ExtendWith;
32+
33+
import static org.hamcrest.MatcherAssert.assertThat;
34+
import static org.hamcrest.Matchers.is;
35+
import static org.junit.jupiter.api.Assertions.assertFalse;
36+
import static org.junit.jupiter.api.Assertions.assertTrue;
37+
38+
@ExtendWith(WorkDirExtension.class)
39+
public class CachingHttpContentFactoryTest
40+
{
41+
public WorkDir workDir;
42+
private ArrayByteBufferPool.Tracking trackingPool;
43+
private ByteBufferPool.Sized sizedPool;
44+
45+
@BeforeEach
46+
public void setUp()
47+
{
48+
trackingPool = new ArrayByteBufferPool.Tracking();
49+
sizedPool = new ByteBufferPool.Sized(trackingPool);
50+
}
51+
52+
@AfterEach
53+
public void tearDown()
54+
{
55+
assertThat("Leaks: " + trackingPool.dumpLeaks(), trackingPool.getLeaks().size(), is(0));
56+
}
57+
58+
@Test
59+
public void testCannotRetainEvictedContent() throws Exception
60+
{
61+
Path file = Files.writeString(workDir.getEmptyPathDir().resolve("file.txt"), "0123456789abcdefghijABCDEFGHIJ");
62+
ResourceHttpContentFactory resourceHttpContentFactory = new ResourceHttpContentFactory(ResourceFactory.root().newResource(file.getParent()), MimeTypes.DEFAULTS);
63+
CachingHttpContentFactory cachingHttpContentFactory = new CachingHttpContentFactory(resourceHttpContentFactory, sizedPool);
64+
65+
CachingHttpContentFactory.CachingHttpContent content = (CachingHttpContentFactory.CachingHttpContent)cachingHttpContentFactory.getContent("file.txt");
66+
67+
// Empty the cache so 'content' gets released.
68+
cachingHttpContentFactory.flushCache();
69+
70+
assertFalse(content.retain());
71+
72+
// Even if the content cannot be retained, whatever we got from cachingHttpContentFactory must be released.
73+
content.release();
74+
}
75+
76+
@Test
77+
public void testRetainThenEvictContent() throws Exception
78+
{
79+
Path file = Files.writeString(workDir.getEmptyPathDir().resolve("file.txt"), "0123456789abcdefghijABCDEFGHIJ");
80+
ResourceHttpContentFactory resourceHttpContentFactory = new ResourceHttpContentFactory(ResourceFactory.root().newResource(file.getParent()), MimeTypes.DEFAULTS);
81+
CachingHttpContentFactory cachingHttpContentFactory = new CachingHttpContentFactory(resourceHttpContentFactory, sizedPool);
82+
83+
CachingHttpContentFactory.CachingHttpContent content = (CachingHttpContentFactory.CachingHttpContent)cachingHttpContentFactory.getContent("file.txt");
84+
85+
assertTrue(content.retain());
86+
87+
// Empty the cache so 'content' gets released.
88+
cachingHttpContentFactory.flushCache();
89+
90+
ByteArrayOutputStream baos = new ByteArrayOutputStream();
91+
BufferUtil.writeTo(content.getByteBuffer(), baos);
92+
assertThat(baos.toString(StandardCharsets.UTF_8), is("0123456789abcdefghijABCDEFGHIJ"));
93+
// Pair the explicit retain that succeeded.
94+
content.release();
95+
96+
// Even if the content cannot be retained, whatever we got from cachingHttpContentFactory must be released.
97+
content.release();
98+
}
99+
}

0 commit comments

Comments
 (0)