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

Needless META-INF/resources and web-fragment.xml mounts #10164

Closed
gregw opened this issue Jul 27, 2023 · 15 comments · Fixed by #10170
Closed

Needless META-INF/resources and web-fragment.xml mounts #10164

gregw opened this issue Jul 27, 2023 · 15 comments · Fixed by #10170
Assignees

Comments

@gregw
Copy link
Contributor

gregw commented Jul 27, 2023

Jetty version(s)
12

Enhancement Description

If you deploy the ee10-demo-async-rest
and dump the server, you see that the ResourceFactory for the context contains a META-INF/resources and META-INF/web-fragment.xml entries for every jar file, even though they do not exist.

Either we should not mount these resources if they do not exist.... or if we are caching non-existence, we should exclude them from a dump.

|     += oejur.ResourceFactoryInternals$LifeCycle@22d7b4f8{STARTED} - STARTED
|     |  +> mounts size=30
|     |     +> Mount[uri=jar:file:///home/gregw/src/jetty-12.0.x/jetty-home/target/jetty-home/lib/jakarta.servlet-api-6.0.0.jar!/,root=jar:file:///home/gregw/src/jetty-12.0.x/jetty-home/target/jetty-home/lib/jakarta.servlet-api-6.0.0.jar!/META-INF/resources]
|     |     +> Mount[uri=jar:file:///home/gregw/src/jetty-12.0.x/jetty-home/target/jetty-home/lib/jakarta.servlet-api-6.0.0.jar!/,root=jar:file:///home/gregw/src/jetty-12.0.x/jetty-home/target/jetty-home/lib/jakarta.servlet-api-6.0.0.jar!/META-INF/web-fragment.xml]
|     |     +> Mount[uri=jar:file:///home/gregw/src/jetty-12.0.x/jetty-home/target/jetty-home/lib/jakarta.servlet-api-6.0.0.jar!/,root=jar:file:///home/gregw/src/jetty-12.0.x/jetty-home/target/jetty-home/lib/jakarta.servlet-api-6.0.0.jar!/]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-alpn-client-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-alpn-client-12.0.0-SNAPSHOT.jar!/META-INF/resources]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-alpn-client-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-alpn-client-12.0.0-SNAPSHOT.jar!/META-INF/web-fragment.xml]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-alpn-client-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-alpn-client-12.0.0-SNAPSHOT.jar!/]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-client-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-client-12.0.0-SNAPSHOT.jar!/META-INF/resources]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-client-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-client-12.0.0-SNAPSHOT.jar!/META-INF/web-fragment.xml]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-client-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-client-12.0.0-SNAPSHOT.jar!/]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-ee10-demo-async-rest-jar-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-ee10-demo-async-rest-jar-12.0.0-SNAPSHOT.jar!/META-INF/resources/]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-ee10-demo-async-rest-jar-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-ee10-demo-async-rest-jar-12.0.0-SNAPSHOT.jar!/META-INF/web-fragment.xml]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-ee10-demo-async-rest-jar-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-ee10-demo-async-rest-jar-12.0.0-SNAPSHOT.jar!/]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-http-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-http-12.0.0-SNAPSHOT.jar!/META-INF/resources]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-http-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-http-12.0.0-SNAPSHOT.jar!/META-INF/web-fragment.xml]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-http-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-http-12.0.0-SNAPSHOT.jar!/]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-io-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-io-12.0.0-SNAPSHOT.jar!/META-INF/resources]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-io-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-io-12.0.0-SNAPSHOT.jar!/META-INF/web-fragment.xml]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-io-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-io-12.0.0-SNAPSHOT.jar!/]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-slf4j-impl-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-slf4j-impl-12.0.0-SNAPSHOT.jar!/META-INF/resources]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-slf4j-impl-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-slf4j-impl-12.0.0-SNAPSHOT.jar!/META-INF/web-fragment.xml]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-slf4j-impl-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-slf4j-impl-12.0.0-SNAPSHOT.jar!/]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-util-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-util-12.0.0-SNAPSHOT.jar!/META-INF/resources]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-util-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-util-12.0.0-SNAPSHOT.jar!/META-INF/web-fragment.xml]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-util-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-util-12.0.0-SNAPSHOT.jar!/]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-util-ajax-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-util-ajax-12.0.0-SNAPSHOT.jar!/META-INF/resources]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-util-ajax-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-util-ajax-12.0.0-SNAPSHOT.jar!/META-INF/web-fragment.xml]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-util-ajax-12.0.0-SNAPSHOT.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/jetty-util-ajax-12.0.0-SNAPSHOT.jar!/]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/slf4j-api-2.0.7.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/slf4j-api-2.0.7.jar!/META-INF/resources]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/slf4j-api-2.0.7.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/slf4j-api-2.0.7.jar!/META-INF/web-fragment.xml]
|     |     +> Mount[uri=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/slf4j-api-2.0.7.jar!/,root=jar:file:///tmp/jetty-0_0_0_0-8080-ee10-demo-async-rest_war-_ee10-demo-async-rest-any-18389751907668110065/webapp/WEB-INF/lib/slf4j-api-2.0.7.jar!/]
|     +- org.eclipse.jetty.util.resource.ResourceFactory$2@4b8729ff

@gregw
Copy link
Contributor Author

gregw commented Jul 27, 2023

Most jar files are not going to have either META-INF/resources nor web-fragments.xml in them. It feels highly inefficient that we are mounting all these jars that will already have been opened by the class loader.

Could we use something like context.getClassLoader().getResources(String) to find all the fragments and resources without needing to mount the jar files just to check? I know it gives a horrible Enumeration of URLs, but we could at least use that as a cheap existence check before mounting the jar ourselves?

At the very very least, can't we unmount the jars that have nothing that we use so we can free the memory used to load them?

@janbartel when we scan a jar for annotations, do we use these mounted resources or does ASM do it's own thing?

@janbartel
Copy link
Contributor

When we scan a Resource that is a jar we now use it as a Path and do a try-with-resources like this:

 try (Stream<Path> classStream = Files.walk(path))

@gregw
Copy link
Contributor Author

gregw commented Jul 27, 2023

Those Paths are probably from a ZipFS, so probably mounted.

Hmmm but we don't end up with all those classes mounted on the resource factory? So something different is happening.

Either way, we need to see if we can be more efficient here

@janbartel
Copy link
Contributor

NB If we want to get rid of the Resources that we keep around (in the context MetaData), we need a different mechanism that remembers the ordering and exclusions of jars on the container classpath and the webapp classpath.

@lorban
Copy link
Contributor

lorban commented Jul 27, 2023

Actually, each jar file is only mounted once, but there is one mount object created for each Resource that is requested via ResourceFactory.newResource() to make a poor man's counter; that could be optimized to use a real counter instead so that you would only see 10 mounts in your example.

If we don't need to keep the resources around after the jars have been scanned, we should change the scope of the ResourceFactory, for instance by creating a Closeable one for the duration of the scan instead of using the one bound to WebAppContext.

@gregw
Copy link
Contributor Author

gregw commented Jul 27, 2023

I still think we should investigate if we can get resources from the classloader. Even 1 mount means that the jar is expanded into memory.

@joakime
Copy link
Contributor

joakime commented Jul 27, 2023

Even 1 mount means that the jar is expanded into memory.

ZipFs works via the Files.newByteChannel(zipfile, READ) mechanism (a SeekableByteChannel)
It doesn't load the entire jar in memory.
It will, however, maintain the zip/jar central directory in memory (essentially the map of named entries to their locations in the zip/jar)

@gregw
Copy link
Contributor Author

gregw commented Jul 27, 2023

So that's good....

But still I don't understand why there are entries for resources that do not exist?

@lorban
Copy link
Contributor

lorban commented Jul 27, 2023

That's an inefficiency of the current implementation: multiple Mount objects are created as they lack a resource counter. But they all point to a single real mount.

ResourceFactory.newResource() was called for jar:file:...x.jar!/META-INF/resources, jar:file:...x.jar!/META-INF/web-fragment.xml and jar:file:...x.jar!/ so the jar file is mounted once, 3 Mount objects are created, and once all 3 Mount instances are closed the jar gets unmounted. We could use a single Mount with a resource counter instead.

@joakime
Copy link
Contributor

joakime commented Jul 28, 2023

@lorban the Mount logic in HEAD seems sane.

I added the a testcase to prove this out in PR #10170 (a testcase that passes btw)

Copy/Pasted here ...

    /**
     * When mounting multiple points within the same JAR, only
     * 1 mount should be created, but have reference counts
     * tracked separately.
     */
    @Test
    public void testMountByJarName()
    {
        Path jarPath = MavenPaths.findTestResourceFile("jar-file-resource.jar");
        URI uriRoot = URI.create("jar:" + jarPath.toUri().toASCIIString() + "!/"); // root
        URI uriRez = URI.create("jar:" + jarPath.toUri().toASCIIString() + "!/rez/"); // dir
        URI uriDeep = URI.create("jar:" + jarPath.toUri().toASCIIString() + "!/rez/deep/"); // dir
        URI uriZzz = URI.create("jar:" + jarPath.toUri().toASCIIString() + "!/rez/deep/zzz"); // file

        try (ResourceFactory.Closeable resourceFactory = ResourceFactory.closeable())
        {
            Resource resRoot = resourceFactory.newResource(uriRoot);
            Resource resRez = resourceFactory.newResource(uriRez);
            Resource resDeep = resourceFactory.newResource(uriDeep);
            Resource resZzz = resourceFactory.newResource(uriZzz);

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

@joakime
Copy link
Contributor

joakime commented Jul 28, 2023

Looks like the dump is showing the tracked mounts of the ResourceFactory, not the tracked mounts of the FileSystemPool.

The naming in the dump should be cleaned up.
Perhaps "references" instead of "mounts", and only showing the URI used via ResourceFactory.newResource() in the output?

The excess entries is a different effort, I'll work on that too.

@gregw
Copy link
Contributor Author

gregw commented Jul 28, 2023

Maybe only dump "references" that exist?

@joakime
Copy link
Contributor

joakime commented Jul 28, 2023

Also pushed a new PR to fix the usages of Resource in the MetaInfConfiguration class.
See PR #10171

joakime added a commit that referenced this issue Jul 31, 2023
#10170)

* Issue #10164 - test to prove out FSPool mount vs reference count logic
* Issue #10164 - improve ResourceFactory.LifeCycle.dump behavior
@joakime joakime moved this to 🏗 In progress in Jetty 12.0.1 - FROZEN Aug 3, 2023
joakime added a commit that referenced this issue Aug 3, 2023
Ensure JPMS and TLDs can coexist happily
Better method naming per reviews
Reworking MetaInfConfiguration with newer Java techniques in mind
@joakime
Copy link
Contributor

joakime commented Oct 23, 2023

Work in branch https://github.com/jetty/jetty.project/compare/fix/12.0.x/metainf-overhaul will be presented as a PR for 12.0.4

@joakime
Copy link
Contributor

joakime commented Jan 24, 2024

Closing as completed already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: ✅ Done
4 participants