From 35d58fc293f296efc8035956942259a74fa2aba2 Mon Sep 17 00:00:00 2001
From: Ludovic Orban <lorban@bitronix.be>
Date: Mon, 13 Jan 2025 13:11:15 +0100
Subject: [PATCH 1/5] #12681 atomically retain the buffer with a check that it
still is in the cache, otherwise delegate to the wrapped content
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
---
.../content/CachingHttpContentFactory.java | 28 ++++++++++++++++---
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/content/CachingHttpContentFactory.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/content/CachingHttpContentFactory.java
index 8b13f72ab513..5ed65c7ffc49 100644
--- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/content/CachingHttpContentFactory.java
+++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/content/CachingHttpContentFactory.java
@@ -354,19 +354,39 @@ public String getKey()
@Override
public void writeTo(Content.Sink sink, long offset, long length, Callback callback)
{
+ boolean retained = false;
try
{
- _buffer.retain();
- sink.write(true, BufferUtil.slice(_buffer.getByteBuffer(), (int)offset, (int)length), Callback.from(_buffer::release, callback));
+ retained = retain();
+ if (retained)
+ sink.write(true, BufferUtil.slice(_buffer.getByteBuffer(), Math.toIntExact(offset), Math.toIntExact(length)), Callback.from(this::release, callback));
+ else
+ getWrapped().writeTo(sink, offset, length, callback);
}
catch (Throwable x)
{
- // BufferUtil.slice() may fail if offset and/or length are out of bounds.
- _buffer.release();
+ // BufferUtil.slice() may fail if offset and/or length are out of bounds,
+ // Math.toIntExact may fail too if offset or length are > Integer.MAX_VALUE.
+ if (retained)
+ release();
callback.failed(x);
}
}
+ /**
+ * Atomically checks that this content still is in cache (so it hasn't been released yet and is still usable) and retain
+ * its internal buffer if it is.
+ * @return true if this content can be used and has been retained, false otherwise.
+ */
+ private boolean retain()
+ {
+ return _cache.computeIfPresent(_cacheKey, (s, cachingHttpContent) ->
+ {
+ _buffer.retain();
+ return cachingHttpContent;
+ }) != null;
+ }
+
@Override
public void release()
{
From c074a30493a97273f59bc93ea2ffc22604f0a747 Mon Sep 17 00:00:00 2001
From: Ludovic Orban <lorban@bitronix.be>
Date: Mon, 13 Jan 2025 13:16:53 +0100
Subject: [PATCH 2/5] #12681 remove invalid comment
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
---
.../eclipse/jetty/http/content/CachingHttpContentFactory.java | 1 -
1 file changed, 1 deletion(-)
diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/content/CachingHttpContentFactory.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/content/CachingHttpContentFactory.java
index 5ed65c7ffc49..8b506132ffe0 100644
--- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/content/CachingHttpContentFactory.java
+++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/content/CachingHttpContentFactory.java
@@ -230,7 +230,6 @@ public HttpContent getContent(String path) throws IOException
if (!isCacheable(httpContent))
return httpContent;
- // The re-mapping function may be run multiple times by compute.
AtomicBoolean added = new AtomicBoolean();
cachingHttpContent = _cache.computeIfAbsent(path, key ->
{
From 00c249f8dd4bd92f2fe41fea8e648db07c95ebca Mon Sep 17 00:00:00 2001
From: Ludovic Orban <lorban@bitronix.be>
Date: Mon, 13 Jan 2025 17:40:50 +0100
Subject: [PATCH 3/5] #12681 add tests
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
---
.../CachingHttpContentFactoryTest.java | 101 ++++++++++++++++++
1 file changed, 101 insertions(+)
create mode 100644 jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/content/CachingHttpContentFactoryTest.java
diff --git a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/content/CachingHttpContentFactoryTest.java b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/content/CachingHttpContentFactoryTest.java
new file mode 100644
index 000000000000..9fdf559966e2
--- /dev/null
+++ b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/content/CachingHttpContentFactoryTest.java
@@ -0,0 +1,101 @@
+//
+// ========================================================================
+// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others.
+//
+// This program and the accompanying materials are made available under the
+// terms of the Eclipse Public License v. 2.0 which is available at
+// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
+// which is available at https://www.apache.org/licenses/LICENSE-2.0.
+//
+// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
+// ========================================================================
+//
+
+package org.eclipse.jetty.http.content;
+
+import java.io.ByteArrayOutputStream;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+
+import org.eclipse.jetty.http.MimeTypes;
+import org.eclipse.jetty.io.ArrayByteBufferPool;
+import org.eclipse.jetty.io.ByteBufferPool;
+import org.eclipse.jetty.io.Content;
+import org.eclipse.jetty.toolchain.test.jupiter.WorkDir;
+import org.eclipse.jetty.toolchain.test.jupiter.WorkDirExtension;
+import org.eclipse.jetty.util.Blocker;
+import org.eclipse.jetty.util.resource.ResourceFactory;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+
+@ExtendWith(WorkDirExtension.class)
+public class CachingHttpContentFactoryTest
+{
+ public WorkDir workDir;
+ private ArrayByteBufferPool.Tracking trackingPool;
+ private ByteBufferPool.Sized sizedPool;
+
+ @BeforeEach
+ public void setUp()
+ {
+ trackingPool = new ArrayByteBufferPool.Tracking();
+ sizedPool = new ByteBufferPool.Sized(trackingPool);
+ }
+
+ @AfterEach
+ public void tearDown()
+ {
+ assertThat("Leaks: " + trackingPool.dumpLeaks(), trackingPool.getLeaks().size(), is(0));
+ }
+
+ @Test
+ public void testWriteEvictedContent() throws Exception
+ {
+ Path file = Files.writeString(workDir.getEmptyPathDir().resolve("file.txt"), "0123456789abcdefghijABCDEFGHIJ");
+ ResourceHttpContentFactory resourceHttpContentFactory = new ResourceHttpContentFactory(ResourceFactory.root().newResource(file.getParent()), MimeTypes.DEFAULTS, sizedPool);
+ CachingHttpContentFactory cachingHttpContentFactory = new CachingHttpContentFactory(resourceHttpContentFactory, sizedPool);
+
+ HttpContent content = cachingHttpContentFactory.getContent("file.txt");
+
+ // Empty the cache so 'content' gets released.
+ cachingHttpContentFactory.flushCache();
+
+ ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ try (Blocker.Callback cb = Blocker.callback())
+ {
+ content.writeTo(Content.Sink.from(baos), 0L, -1L, cb);
+ cb.block();
+ }
+ assertThat(baos.toString(StandardCharsets.UTF_8), is("0123456789abcdefghijABCDEFGHIJ"));
+ }
+
+ @Test
+ public void testEvictBetweenWriteToAndSinkWrite() throws Exception
+ {
+ Path file = Files.writeString(workDir.getEmptyPathDir().resolve("file.txt"), "0123456789abcdefghijABCDEFGHIJ");
+ ResourceHttpContentFactory resourceHttpContentFactory = new ResourceHttpContentFactory(ResourceFactory.root().newResource(file.getParent()), MimeTypes.DEFAULTS, sizedPool);
+ CachingHttpContentFactory cachingHttpContentFactory = new CachingHttpContentFactory(resourceHttpContentFactory, sizedPool);
+
+ HttpContent content = cachingHttpContentFactory.getContent("file.txt");
+
+ ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ try (Blocker.Callback cb = Blocker.callback())
+ {
+ content.writeTo((last, byteBuffer, callback) ->
+ {
+ // Empty the cache so 'content' gets released.
+ cachingHttpContentFactory.flushCache();
+
+ Content.Sink.from(baos).write(last, byteBuffer, callback);
+ }, 0L, -1L, cb);
+ cb.block();
+ }
+ assertThat(baos.toString(StandardCharsets.UTF_8), is("0123456789abcdefghijABCDEFGHIJ"));
+ }
+}
From 65de5a659f1f7d57f59a27359d284b1c4a20b18d Mon Sep 17 00:00:00 2001
From: Ludovic Orban <lorban@bitronix.be>
Date: Tue, 14 Jan 2025 10:43:14 +0100
Subject: [PATCH 4/5] #12681 handle review comments
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
---
.../eclipse/jetty/http/content/CachingHttpContentFactory.java | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/content/CachingHttpContentFactory.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/content/CachingHttpContentFactory.java
index 32393aaa3594..39e1fb237a4a 100644
--- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/content/CachingHttpContentFactory.java
+++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/content/CachingHttpContentFactory.java
@@ -368,7 +368,7 @@ public void writeTo(Content.Sink sink, long offset, long length, Callback callba
boolean retained = false;
try
{
- retained = retain();
+ retained = cachedRetain();
if (retained)
sink.write(true, BufferUtil.slice(_buffer.getByteBuffer(), Math.toIntExact(offset), Math.toIntExact(length)), Callback.from(this::release, callback));
else
@@ -389,7 +389,7 @@ public void writeTo(Content.Sink sink, long offset, long length, Callback callba
* its internal buffer if it is.
* @return true if this content can be used and has been retained, false otherwise.
*/
- private boolean retain()
+ private boolean cachedRetain()
{
return _cache.computeIfPresent(_cacheKey, (s, cachingHttpContent) ->
{
From e2aa7aaf367339e209ec2c1cbb07a0a895cf9ae1 Mon Sep 17 00:00:00 2001
From: Ludovic Orban <lorban@bitronix.be>
Date: Wed, 15 Jan 2025 10:23:22 +0100
Subject: [PATCH 5/5] #12681 tryRetain it is
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
---
.../eclipse/jetty/http/content/CachingHttpContentFactory.java | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/content/CachingHttpContentFactory.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/content/CachingHttpContentFactory.java
index 39e1fb237a4a..b9857a655e19 100644
--- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/content/CachingHttpContentFactory.java
+++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/content/CachingHttpContentFactory.java
@@ -368,7 +368,7 @@ public void writeTo(Content.Sink sink, long offset, long length, Callback callba
boolean retained = false;
try
{
- retained = cachedRetain();
+ retained = tryRetain();
if (retained)
sink.write(true, BufferUtil.slice(_buffer.getByteBuffer(), Math.toIntExact(offset), Math.toIntExact(length)), Callback.from(this::release, callback));
else
@@ -389,7 +389,7 @@ public void writeTo(Content.Sink sink, long offset, long length, Callback callba
* its internal buffer if it is.
* @return true if this content can be used and has been retained, false otherwise.
*/
- private boolean cachedRetain()
+ private boolean tryRetain()
{
return _cache.computeIfPresent(_cacheKey, (s, cachingHttpContent) ->
{