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.

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

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Nov 13, 2023

Don't hold / track mounts from newResource() that would result in a no-op.

@joakime joakime requested a review from gregw November 13, 2023 22:09
@joakime joakime self-assigned this Nov 13, 2023
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

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.

@gregw
Copy link
Contributor

gregw commented Nov 14, 2023

@joakime I have pushed a quick fix:

  • We were already only mounting the root of a jar, but now we also record that as the root Resource
  • mountIfNeeded now checks existing mounts to see if we have already mounted the root Resource. There is probably a much better way of doing this than by string comparison.
  • if we have already mounted the root of a jar URI, then we return null from mountIfNeeded as another mount is not needed.
  • we now don't need special handling for non existent resources. The root is still mounted and the non existent path is forgotten

This almost certainly needs more work/optimisation/cleanup, but I hope it conveys what I think needs to be done.

@joakime joakime merged commit 2b3d811 into jetty-12.0.x Nov 15, 2023
9 checks passed
@joakime joakime deleted the fix/12.0.x/resourcefactory-nomount-on-nonexistent-resource branch November 15, 2023 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants