From fde5b9a6db4d9e8c1adcc92368eb936845807c45 Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 4 Jul 2023 13:25:03 +0200 Subject: [PATCH 1/3] Simplified URLResource cleaner The reference to the InputStream does not need to be soft/weak as a hard reference to it does not prevent the URLResource itself from being collected. Also by making a runnable record, there is one less allocation per input stream creation. --- .../util/resource/URLResourceFactory.java | 24 +++++++++++-------- .../util/resource/UrlResourceFactoryTest.java | 4 ++-- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResourceFactory.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResourceFactory.java index 7ceed6f4240..093c32aa85d 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResourceFactory.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResourceFactory.java @@ -16,8 +16,6 @@ import java.io.IOException; import java.io.InputStream; import java.lang.ref.Cleaner; -import java.lang.ref.Reference; -import java.lang.ref.SoftReference; import java.net.MalformedURLException; import java.net.URI; import java.net.URL; @@ -40,7 +38,7 @@ */ public class URLResourceFactory implements ResourceFactory { - static Consumer> ON_SWEEP_LISTENER; + static Consumer ON_SWEEP_LISTENER; private int connectTimeout = 0; private int readTimeout = 0; private boolean useCaches = true; @@ -223,13 +221,7 @@ public boolean exists() if (in == null) { in = connection.getInputStream(); - Reference ref = new SoftReference<>(in); - CLEANER.register(this, () -> - { - IO.close(ref.get()); - if (ON_SWEEP_LISTENER != null) - ON_SWEEP_LISTENER.accept(ref); - }); + CLEANER.register(this, new InputStreamCleaner(in)); } ret = in != null; } @@ -317,5 +309,17 @@ public String toString() { return String.format("URLResource@%X(%s)", this.uri.hashCode(), this.uri.toASCIIString()); } + + private record InputStreamCleaner(InputStream inputStream) implements Runnable + { + @Override + public void run() + { + // Called when the URLResource that held the same InputStream has been collected + IO.close(inputStream); + if (ON_SWEEP_LISTENER != null) + ON_SWEEP_LISTENER.accept(inputStream); + } + } } } diff --git a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/UrlResourceFactoryTest.java b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/UrlResourceFactoryTest.java index 697dab0410a..71fce512587 100644 --- a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/UrlResourceFactoryTest.java +++ b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/UrlResourceFactoryTest.java @@ -100,9 +100,9 @@ public void testInputStreamCleanedUp() throws Exception AtomicInteger cleanedRefCount = new AtomicInteger(); URLResourceFactory urlResourceFactory = new URLResourceFactory(); - URLResourceFactory.ON_SWEEP_LISTENER = ref -> + URLResourceFactory.ON_SWEEP_LISTENER = in -> { - if (ref != null && ref.get() != null) + if (in != null) cleanedRefCount.incrementAndGet(); }; Resource resource = urlResourceFactory.newResource(jarFileUri.toURL()); From b54394689ec69d71ed23f2d1acc6727be19c2e4e Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 4 Jul 2023 18:58:04 +0200 Subject: [PATCH 2/3] Simplified URLResource cleaner Use AtomicReference for the inputstream, so that if it is taken, then it is nulled and will not be closed. --- .../util/resource/URLResourceFactory.java | 41 ++++++++++++------- .../util/resource/UrlResourceFactoryTest.java | 2 - 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResourceFactory.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResourceFactory.java index 093c32aa85d..ab47e37d171 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResourceFactory.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResourceFactory.java @@ -24,6 +24,7 @@ import java.nio.channels.ReadableByteChannel; import java.nio.file.Path; import java.time.Instant; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import org.eclipse.jetty.util.FileID; @@ -114,7 +115,7 @@ private static class URLResource extends Resource private final int readTimeout; private final boolean useCaches; private URLConnection connection; - private InputStream in = null; + private InputStreamReference inputStreamReference = null; public URLResource(URI uri, int connectTimeout, int readTimeout, boolean useCaches) throws MalformedURLException { @@ -213,24 +214,23 @@ public Resource resolve(String subUriPath) @Override public boolean exists() { - boolean ret = false; try (AutoLock l = lock.lock()) { if (checkConnection()) { - if (in == null) + if (inputStreamReference == null || inputStreamReference.get() == null) { - in = connection.getInputStream(); - CLEANER.register(this, new InputStreamCleaner(in)); + inputStreamReference = new InputStreamReference(connection.getInputStream()); + CLEANER.register(this, inputStreamReference); } - ret = in != null; + return true; } } catch (IOException e) { LOG.trace("IGNORED", e); } - return ret; + return false; } @Override @@ -243,11 +243,12 @@ public InputStream newInputStream() throws IOException try { - if (in != null) + if (inputStreamReference != null) { - InputStream stream = in; - in = null; - return stream; + InputStream stream = inputStreamReference.getAndSet(null); + inputStreamReference = null; + if (stream != null) + return stream; } return connection.getInputStream(); } @@ -310,16 +311,26 @@ public String toString() return String.format("URLResource@%X(%s)", this.uri.hashCode(), this.uri.toASCIIString()); } - private record InputStreamCleaner(InputStream inputStream) implements Runnable + private static class InputStreamReference extends AtomicReference implements Runnable { + public InputStreamReference(InputStream initialValue) + { + super(initialValue); + } + @Override public void run() { // Called when the URLResource that held the same InputStream has been collected - IO.close(inputStream); - if (ON_SWEEP_LISTENER != null) - ON_SWEEP_LISTENER.accept(inputStream); + InputStream in = getAndSet(null); + if (in != null) + { + IO.close(in); + if (ON_SWEEP_LISTENER != null) + ON_SWEEP_LISTENER.accept(in); + } } } + } } diff --git a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/UrlResourceFactoryTest.java b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/UrlResourceFactoryTest.java index 71fce512587..11a2585a91a 100644 --- a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/UrlResourceFactoryTest.java +++ b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/UrlResourceFactoryTest.java @@ -110,8 +110,6 @@ public void testInputStreamCleanedUp() throws Exception Resource webResource = resource.resolve("/web.xml"); assertTrue(webResource.exists()); - webResource = null; - await().atMost(5, TimeUnit.SECONDS).until(() -> { System.gc(); From 1be8a2acccf831cbb35fb877c4d897c64f40a531 Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 5 Jul 2023 10:24:43 +0200 Subject: [PATCH 3/3] Simplified URLResource cleaner Improved tests --- .../util/resource/URLResourceFactory.java | 7 ++-- .../util/resource/UrlResourceFactoryTest.java | 33 ++++++++++++++++++- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResourceFactory.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResourceFactory.java index ab47e37d171..3e406748682 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResourceFactory.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResourceFactory.java @@ -324,11 +324,10 @@ public void run() // Called when the URLResource that held the same InputStream has been collected InputStream in = getAndSet(null); if (in != null) - { IO.close(in); - if (ON_SWEEP_LISTENER != null) - ON_SWEEP_LISTENER.accept(in); - } + + if (ON_SWEEP_LISTENER != null) + ON_SWEEP_LISTENER.accept(in); } } diff --git a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/UrlResourceFactoryTest.java b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/UrlResourceFactoryTest.java index 11a2585a91a..fdd81844f4b 100644 --- a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/UrlResourceFactoryTest.java +++ b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/UrlResourceFactoryTest.java @@ -39,6 +39,7 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -106,9 +107,12 @@ public void testInputStreamCleanedUp() throws Exception cleanedRefCount.incrementAndGet(); }; Resource resource = urlResourceFactory.newResource(jarFileUri.toURL()); - Resource webResource = resource.resolve("/web.xml"); assertTrue(webResource.exists()); + resource = null; + webResource = null; + assertThat(resource, nullValue()); + assertThat(webResource, nullValue()); await().atMost(5, TimeUnit.SECONDS).until(() -> { @@ -117,6 +121,33 @@ public void testInputStreamCleanedUp() throws Exception }); } + @Test + public void testTakenInputStreamNotClosedOnCleanUp() throws Exception + { + Path path = MavenTestingUtils.getTestResourcePath("example.jar"); + URI jarFileUri = URI.create("jar:" + path.toUri().toASCIIString() + "!/WEB-INF/"); + + AtomicInteger cleanedRefCount = new AtomicInteger(); + URLResourceFactory urlResourceFactory = new URLResourceFactory(); + URLResourceFactory.ON_SWEEP_LISTENER = in -> cleanedRefCount.incrementAndGet(); + Resource resource = urlResourceFactory.newResource(jarFileUri.toURL()); + Resource webResource = resource.resolve("/web.xml"); + assertTrue(webResource.exists()); + InputStream in = webResource.newInputStream(); + resource = null; + webResource = null; + assertThat(resource, nullValue()); + assertThat(webResource, nullValue()); + await().atMost(5, TimeUnit.SECONDS).until(() -> + { + System.gc(); + return cleanedRefCount.get() > 0; + }); + + String webXml = IO.toString(in); + assertThat(webXml, is("WEB-INF/web.xml")); + } + @Test public void testFileUrl() throws Exception {