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 c48ef5bfeaa..b9857a655e1 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 @@ -242,7 +242,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 -> { @@ -366,19 +365,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 = tryRetain(); + 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 tryRetain() + { + return _cache.computeIfPresent(_cacheKey, (s, cachingHttpContent) -> + { + _buffer.retain(); + return cachingHttpContent; + }) != null; + } + @Override public void release() { 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 00000000000..9fdf559966e --- /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")); + } +}