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

Jump to bottom

Simplified URLResource cleaner #10061

Merged
merged 3 commits into from
Jul 5, 2023
Merged

Simplified URLResource cleaner #10061

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fde5b9a
Simplified URLResource cleaner
gregw Jul 4, 2023
b543946
Simplified URLResource cleaner
gregw Jul 4, 2023
1be8a2a
Simplified URLResource cleaner
gregw Jul 5, 2023
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
54 changes: 34 additions & 20 deletions jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResourceFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -26,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;
Expand All @@ -40,7 +39,7 @@
*/
public class URLResourceFactory implements ResourceFactory
{
static Consumer<Reference<InputStream>> ON_SWEEP_LISTENER;
static Consumer<InputStream> ON_SWEEP_LISTENER;
private int connectTimeout = 0;
private int readTimeout = 0;
private boolean useCaches = true;
Expand Down Expand Up @@ -116,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
{
Expand Down Expand Up @@ -215,30 +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();
Reference<InputStream> ref = new SoftReference<>(in);
CLEANER.register(this, () ->
{
IO.close(ref.get());
if (ON_SWEEP_LISTENER != null)
ON_SWEEP_LISTENER.accept(ref);
});
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
Expand All @@ -251,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();
}
Expand Down Expand Up @@ -317,5 +310,26 @@ public String toString()
{
return String.format("URLResource@%X(%s)", this.uri.hashCode(), this.uri.toASCIIString());
}

private static class InputStreamReference extends AtomicReference<InputStream> implements Runnable
{
public InputStreamReference(InputStream initialValue)
{
super(initialValue);
}

@Override
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);
}
}

}
}
37 changes: 33 additions & 4 deletions ...core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/UrlResourceFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -100,17 +101,18 @@ 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());

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(() ->
{
Expand All @@ -119,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
{
Expand Down