Skip to content

Commit

Permalink
Don't track mounts for newResource() that doesn't exist (#10886)
Browse files Browse the repository at this point in the history 
* Don't track mounts for newResource() that doesn't exist.
* Cannot dereference if not tracking both resources that use the JAR.
* Only mount the root.  Check if root already mounted in mountIfNeeded

---------

Co-authored-by: gregw <[email protected]>
  • Loading branch information
@joakime @gregw
joakime and gregw authored Nov 15, 2023
1 parent b436c85 commit 2b3d811
Show file tree
Hide file tree
Showing 4 changed files with 189 additions and 14 deletions.
20 changes: 14 additions & 6 deletions jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileSystemPool.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,27 +108,28 @@ Mount mount(URI uri) throws IOException
throw new IllegalArgumentException("not an supported scheme: " + uri);

FileSystem fileSystem = null;
URI jarURIRoot = toJarURIRoot(uri);
try (AutoLock ignore = poolLock.lock())
{
try
{
fileSystem = FileSystems.newFileSystem(uri, ENV_MULTIRELEASE_RUNTIME);
fileSystem = FileSystems.newFileSystem(jarURIRoot, ENV_MULTIRELEASE_RUNTIME);
if (LOG.isDebugEnabled())
LOG.debug("Mounted new FS {}", uri);
LOG.debug("Mounted new FS {}", jarURIRoot);
}
catch (FileSystemAlreadyExistsException fsaee)
{
fileSystem = Paths.get(uri).getFileSystem();
fileSystem = Paths.get(jarURIRoot).getFileSystem();
if (LOG.isDebugEnabled())
LOG.debug("Using existing FS {}", uri);
LOG.debug("Using existing FS {}", jarURIRoot);
}
catch (ProviderNotFoundException pnfe)
{
throw new IllegalArgumentException("Unable to mount FileSystem from unsupported URI: " + uri, pnfe);
throw new IllegalArgumentException("Unable to mount FileSystem from unsupported URI: " + jarURIRoot, pnfe);
}
// use root FS URI so that pool key/release/sweep is sane
URI rootURI = fileSystem.getPath("/").toUri();
Mount mount = new Mount(rootURI, new MountedPathResource(uri));
Mount mount = new Mount(rootURI, new MountedPathResource(jarURIRoot));
retain(rootURI, fileSystem, mount);
return mount;
}
Expand All @@ -139,6 +140,13 @@ Mount mount(URI uri) throws IOException
}
}

private URI toJarURIRoot(URI uri)
{
String rawURI = uri.toASCIIString();
int idx = rawURI.indexOf("!/");
return URI.create(rawURI.substring(0, idx + 2));
}

private void unmount(URI fsUri)
{
try (AutoLock ignore = poolLock.lock())
Expand Down
30 changes: 28 additions & 2 deletions ...re/jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceFactoryInternals.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,23 +127,37 @@ static boolean isSupported(String str)
return RESOURCE_FACTORIES.getBest(str) != null;
}

static class Closeable implements ResourceFactory.Closeable
interface Tracking
{
int getTrackingCount();
}

static class Closeable implements ResourceFactory.Closeable, Tracking
{
private boolean closed = false;
private final CompositeResourceFactory _compositeResourceFactory = new CompositeResourceFactory();

@Override
public Resource newResource(URI uri)
{
if (closed)
throw new IllegalStateException("Unable to create new Resource on closed ResourceFactory");
return _compositeResourceFactory.newResource(uri);
}

@Override
public void close()
{
closed = true;
for (FileSystemPool.Mount mount : _compositeResourceFactory.getMounts())
IO.close(mount);
_compositeResourceFactory.clearMounts();
}

public int getTrackingCount()
{
return _compositeResourceFactory.getMounts().size();
}
}

static class LifeCycle extends AbstractLifeCycle implements ResourceFactory.LifeCycle
Expand All @@ -153,6 +167,7 @@ static class LifeCycle extends AbstractLifeCycle implements ResourceFactory.Life
@Override
public Resource newResource(URI uri)
{
// TODO: add check that LifeCycle is started before allowing this method to be used?
return _compositeResourceFactory.newResource(uri);
}

Expand Down Expand Up @@ -227,14 +242,25 @@ public Resource newResource(URI uri)
*
* @param uri The URI to mount that may require a FileSystem (e.g. "jar:file://tmp/some.jar!/directory/file.txt")
* @return A reference counted {@link FileSystemPool.Mount} for that file system or null. Callers should call
* {@link FileSystemPool.Mount#close()} once they no longer require any resources from a mounted resource.
* {@link FileSystemPool.Mount#close()} once they no longer require this specific Mount.
* @throws IllegalArgumentException If the uri could not be mounted.
*/
private FileSystemPool.Mount mountIfNeeded(URI uri)
{
// do not mount if it is not a jar URI
String scheme = uri.getScheme();
if (!"jar".equalsIgnoreCase(scheme))
return null;

// Do not mount if we have already mounted
// TODO there is probably a better way of doing this other than string comparisons
String uriString = uri.toASCIIString();
for (FileSystemPool.Mount mount : _mounts)
{
if (uriString.startsWith(mount.root().toString()))
return null;
}

try
{
return FileSystemPool.INSTANCE.mount(uri);
Expand Down
9 changes: 3 additions & 6 deletions ...ore/jetty-util/src/test/java/org/eclipse/jetty/util/resource/MountedPathResourceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,7 @@ public void testMountByJarNameClosable()

assertThat(FileSystemPool.INSTANCE.mounts().size(), is(1));
int mountCount = FileSystemPool.INSTANCE.getReferenceCount(uriRoot);
assertThat(mountCount, is(4));
assertThat(mountCount, is(1));
}

assertThat(FileSystemPool.INSTANCE.mounts().size(), is(0));
Expand Down Expand Up @@ -433,13 +433,10 @@ public void testMountByJarNameLifeCycle() throws Exception

assertThat(FileSystemPool.INSTANCE.mounts().size(), is(1));
int mountCount = FileSystemPool.INSTANCE.getReferenceCount(uriRoot);
assertThat(mountCount, is(4));
assertThat(mountCount, is(1));
String dump = resourceFactory.dump();
assertThat(dump, containsString("newResourceReferences size=4"));
assertThat(dump, containsString("newResourceReferences size=1"));
assertThat(dump, containsString(uriRoot.toASCIIString()));
assertThat(dump, containsString(uriRez.toASCIIString()));
assertThat(dump, containsString(uriDeep.toASCIIString()));
assertThat(dump, containsString(uriZzz.toASCIIString()));
}
finally
{
Expand Down
144 changes: 144 additions & 0 deletions jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/PathResourceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import java.net.URI;
import java.nio.channels.ReadableByteChannel;
import java.nio.charset.StandardCharsets;
import java.nio.file.ClosedFileSystemException;
import java.nio.file.FileSystem;
import java.nio.file.FileSystemException;
import java.nio.file.FileSystems;
Expand Down Expand Up @@ -54,6 +55,7 @@
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assumptions.assumeTrue;

Expand Down Expand Up @@ -236,6 +238,148 @@ public void testGetPathTo(WorkDir workDir) throws IOException
}
}

@Test
public void testJarMountNonExistent(WorkDir workDir) throws IOException
{
Path tmpPath = workDir.getEmptyPathDir();
Path testJar = tmpPath.resolve("test.jar");

URI jarUri = URIUtil.uriJarPrefix(testJar.toUri(), "!/");

Map<String, String> env = new HashMap<>();
env.put("create", "true");
try (FileSystem zipfs = FileSystems.newFileSystem(jarUri, env))
{
zipfs.getPath("/");
}
try (ResourceFactory.Closeable resourceFactory = ResourceFactory.closeable())
{
Resource resBadDir = resourceFactory.newResource(jarUri.toASCIIString() + "does-not-exist/");
assertNull(resBadDir);
Resource resBadFile = resourceFactory.newResource(jarUri.toASCIIString() + "bad/file.txt");
assertNull(resBadFile);

if (resourceFactory instanceof ResourceFactoryInternals.Tracking tracking)
{
assertThat(tracking.getTrackingCount(), is(1));
}
}
}

@Test
public void testMountsForSameJar(WorkDir workDir) throws IOException
{
Path tmpPath = workDir.getEmptyPathDir();
Path testJar = tmpPath.resolve("test.jar");

Map<String, String> env = new HashMap<>();
env.put("create", "true");

URI jarUri = URIUtil.uriJarPrefix(testJar.toUri(), "!/");
try (FileSystem zipfs = FileSystems.newFileSystem(jarUri, env))
{
Path root = zipfs.getPath("/");
Files.writeString(root.resolve("one.txt"), "Contents of one.txt", StandardCharsets.UTF_8);

Path dir = root.resolve("datainf");
Files.createDirectory(dir);
Files.writeString(dir.resolve("two.txt"), "Contents of two.txt", StandardCharsets.UTF_8);
}

try (ResourceFactory.Closeable resourceFactory = ResourceFactory.closeable())
{
Resource oneTxt = resourceFactory.newResource(jarUri.toASCIIString() + "one.txt");
assertTrue(Resources.isReadableFile(oneTxt));
Resource twoTxt = resourceFactory.newResource(jarUri.toASCIIString() + "datainf/two.txt");
assertTrue(Resources.isReadableFile(twoTxt));

if (resourceFactory instanceof ResourceFactoryInternals.Tracking tracking)
{
assertThat(tracking.getTrackingCount(), is(1));
}
}
}

@Test
public void testMountsForSameJarDifferentResourceFactories(WorkDir workDir) throws IOException
{
Path tmpPath = workDir.getEmptyPathDir();
Path testJar = tmpPath.resolve("test.jar");

Map<String, String> env = new HashMap<>();
env.put("create", "true");

URI jarUri = URIUtil.uriJarPrefix(testJar.toUri(), "!/");
try (FileSystem zipfs = FileSystems.newFileSystem(jarUri, env))
{
Path root = zipfs.getPath("/");
Files.writeString(root.resolve("one.txt"), "Contents of one.txt", StandardCharsets.UTF_8);

Path dir = root.resolve("datainf");
Files.createDirectory(dir);
Files.writeString(dir.resolve("two.txt"), "Contents of two.txt", StandardCharsets.UTF_8);
}

assertThat(FileSystemPool.INSTANCE.mounts(), is(empty()));

try (ResourceFactory.Closeable resourceFactory1 = ResourceFactory.closeable();
ResourceFactory.Closeable resourceFactory2 = ResourceFactory.closeable())
{
Resource oneTxt = resourceFactory1.newResource(jarUri.toASCIIString() + "one.txt");
assertTrue(Resources.isReadableFile(oneTxt));

Resource oneTxt2 = resourceFactory1.newResource(jarUri.toASCIIString() + "one.txt");
assertTrue(Resources.isReadableFile(oneTxt));

Resource twoTxt = resourceFactory2.newResource(jarUri.toASCIIString() + "datainf/two.txt");
assertTrue(Resources.isReadableFile(twoTxt));

assertThat("Should see only 1 FS Mount", FileSystemPool.INSTANCE.mounts().size(), is(1));

if (resourceFactory1 instanceof ResourceFactoryInternals.Tracking tracking)
{
assertThat(tracking.getTrackingCount(), is(1));
}

if (resourceFactory2 instanceof ResourceFactoryInternals.Tracking tracking)
{
assertThat(tracking.getTrackingCount(), is(1));
}

// Close Resource Factory 1
resourceFactory1.close();

if (resourceFactory1 instanceof ResourceFactoryInternals.Tracking tracking)
{
assertThat(tracking.getTrackingCount(), is(0));
}

// Resource one still works because factory 2 is holding filesystem open
assertThat(IO.toString(oneTxt.newInputStream()), is("Contents of one.txt"));

// should not be able to use closed ResourceFactory.Closable
assertThrows(IllegalStateException.class, () -> resourceFactory1.newResource(jarUri.toASCIIString() + "one.txt"));

assertThat("Should see only 1 FS Mount", FileSystemPool.INSTANCE.mounts().size(), is(1));

Resource oneAlt = resourceFactory2.newResource(jarUri.toASCIIString() + "one.txt");
assertTrue(Resources.isReadableFile(oneAlt));

// Close Resource Factory 2
resourceFactory2.close();

// Neither Resource one nor two works because filesystem is now closed
assertThrows(ClosedFileSystemException.class, oneTxt::newInputStream);
assertThrows(ClosedFileSystemException.class, oneTxt2::newInputStream);

assertThat("Should see only 0 FS Mount", FileSystemPool.INSTANCE.mounts().size(), is(0));
}
finally
{
assertThat(FileSystemPool.INSTANCE.mounts(), is(empty()));
}
}

@Test
public void testJarFileIsAliasFile(WorkDir workDir) throws IOException
{
Expand Down

0 comments on commit 2b3d811

Please sign in to comment.