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

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

Merged
merged 6 commits into from
Nov 15, 2023
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0b30e3e
Don't track mounts for newResource() that doesn't exist.
joakime Nov 13, 2023
d01b55a
Cannot dereference if not tracking both resources that use the JAR.
joakime Nov 13, 2023
c0e3c5b
Only mount the root. Check if root already mounted in mountIfNeeded
gregw Nov 14, 2023
89b1ed9
simplified produceChunk
gregw Nov 14, 2023
4c85365
Don't track mounts for newResource() that doesn't exist.
joakime Nov 14, 2023
62c6b6e
improved test
gregw Nov 14, 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
18 changes: 13 additions & 5 deletions jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileSystemPool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,23 +108,24 @@
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);
Dismissed Show dismissed Hide dismissed
Dismissed Show dismissed Hide dismissed
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();
Dismissed Show dismissed Hide dismissed
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();
Expand All @@ -139,6 +140,13 @@
}
}

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
21 changes: 19 additions & 2 deletions ...re/jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceFactoryInternals.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,12 @@ static boolean isSupported(String str)
return RESOURCE_FACTORIES.getBest(str) != null;
}

static class Closeable implements ResourceFactory.Closeable
interface Mountable
{
List<FileSystemPool.Mount> getMounts();
}

static class Closeable implements ResourceFactory.Closeable, Mountable
{
private final CompositeResourceFactory _compositeResourceFactory = new CompositeResourceFactory();

Expand All @@ -144,6 +149,11 @@ public void close()
IO.close(mount);
_compositeResourceFactory.clearMounts();
}

public List<FileSystemPool.Mount> getMounts()
{
return _compositeResourceFactory.getMounts();
}
}

static class LifeCycle extends AbstractLifeCycle implements ResourceFactory.LifeCycle
Expand Down Expand Up @@ -210,8 +220,15 @@ public Resource newResource(URI uri)
FileSystemPool.Mount mount = mountIfNeeded(uri);
if (mount != null)
{
Resource res = resourceFactory.newResource(uri);
if (res == null)
{
mount.close(); // decrement
return null;
}
_mounts.add(mount);
onMounted(mount, uri);
return res;
}
}
return resourceFactory.newResource(uri);
Expand All @@ -227,7 +244,7 @@ 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)
Expand Down
40 changes: 40 additions & 0 deletions jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/PathResourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,46 @@ 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");

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("test.txt"), "Contents of test.txt", StandardCharsets.UTF_8);

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

try (ResourceFactory.Closeable resourceFactory = ResourceFactory.closeable())
{
// First reference to Resource in test.jar
Resource resRoot = resourceFactory.newResource(jarUri);
Resource resBadDir = resourceFactory.newResource(jarUri.toASCIIString() + "does-not-exist/");
assertNull(resBadDir);
Resource resBadFile = resourceFactory.newResource(jarUri.toASCIIString() + "bad/file.txt");
assertNull(resBadFile);
// Second reference to Resource in test.jar
Resource resInfoTxt = resourceFactory.newResource(jarUri.toASCIIString() + "datainf/info.txt");
assertTrue(Resources.isReadableFile(resInfoTxt));

if (resourceFactory instanceof ResourceFactoryInternals.Mountable mountable)
{
List<FileSystemPool.Mount> mounts = mountable.getMounts();
assertThat(mounts.size(), is(2));
Copy link
Contributor Author

@joakime joakime Nov 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gregw cannot have this be 1.

As the FileSystemPool is JVM wide, not ResourceFactory specific.
If 2 Resources, in different ResourceFactories access the same JAR these need to be tracked via the Resource -> Mount so that they can be dereferenced and FileSystem closed at the right time. That tracking occurs so that the close() of the ResourceFactory can properly dereference count the FileSystem use.

If we have resRoot cause the mount, and resInfoTxt reuse the mount, then there's not sufficient information to know how to eventually close that mount (especially so when the same jar is referenced as a Resource in 2 different ResourceFactory instances)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joakime if two ResourceFactorys open the same jar file, then each should have it listed as a single mount point. When both factories are closed, then the mount point can be unmounted. How many resources that a given factory creates against its single mount is irrelevant to the reference counting for the close.

Fundamentally, the number of mount points in the factory for:

   Resource foo = factory.newResource("jar:file:///tmp/test.jar!/foo/");
   Resource bar = factory.newResource("jar:file:///tmp/test.jar!/foo/bar.txt");

should be identical to

   Resource foo = factory.newResource("jar:file:///tmp/test.jar!/foo/");
   Resource bar = foo.resolve("bar.txt");

The count of mounts must be independent of how many resources are created against it.

The problem is in mountIfNeeded(URI uri) as it is always mounting, even if not needed. It should be checking existing mounts and not mount again if it is already mounted.

}
} // close dereferences both references to test.jar
}

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