Don't track mounts for newResource() that doesn't exist

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileSystemPool.java

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

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceFactoryInternals.java

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

jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/MountedPathResourceTest.java

@@ -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));
@@ -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
         {

jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/PathResourceTest.java

@@ -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;
@@ -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;
 
@@ -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
     {