Improve Resource use in MetaInfConfiguration #10889

janbartel · 2023-11-14T05:58:47Z

Reduce mounts of jar:file references in MetaInfConfiguration.
Only align META-INF/resources and META-INF/web-fragment.xml Resources with the lifecycle of the WebAppContext
Make scan of .tlds more efficient.

gregw

as I will be away, feel free to dismiss this review once this is fixed

...ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/MetaInfConfiguration.java

joakime

This is about 10% of what PR #10816 does.

This PR doesn't address the other issues with MetaInfConfiguration (that PR #10816 does address)

such as the NPEs present in some of the find methods
the continued use of File / Files / Path APIs
the bad scope of methods
the constant open/close/open/close/etc of Resources for no good reason
the lack of proper use of Resources (note the plural) utility methods
creating Resource objects of jar files themselves, not the contents (there is no reason to do that in this entire class, mount them, lots of code gets simpler! every Resource is a directory!)

...ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/MetaInfConfiguration.java

joakime · 2023-11-14T19:05:52Z

...ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/MetaInfConfiguration.java

+            if (!metaInf.isDirectory())
+                return tlds; //no tlds
+
+            try (Stream<Path> stream = Files.walk(metaInf.getPath()))


Use the Resource API, not Path + Walk. (like the other PR does).

In fact, I just checked, both this PR and my other one does this entire step (scan for TLDs) incorrectly per spec.

For webapps, its a recursive scan for anything *.tld from root.

ignore if found in /WEB-INF/classes/

ignore if found in /WEB-INF/lib/

ignore if ends in / (ie: it's a directory called foo.tld/)

ignore if found in /WEB-INF/tags/ (with exception for implicit.tld)

all other hits are valid.

This can't be correct, as if you look at all previous versions of jetty that are looking for tlds we're using the JarFile class to enumerate all entries in the jar, then picking those that start with META_INF and end with .tld.

I opened Issue #10899 for this (with links to relevant parts of the spec)

...ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/MetaInfConfiguration.java

joakime · 2023-11-14T19:10:06Z

...ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/MetaInfConfiguration.java

     * @return the collection of tlds as url references
     * @throws IOException if unable to scan the jar file
     */
-    private Collection<URL> getTlds(WebAppContext context, URI uri) throws IOException
+    private Collection<URL> getTlds(WebAppContext context, Resource resource) throws IOException
    {
        HashSet<URL> tlds = new HashSet<>();


This is strongly discouraged by Java now. (using URL in a Set or any kind of comparison or hashcode/equals logic)

Well, as we're passing this on to the Apache Jasper implementation we need to support what they want.

The Apache Jasper Implementation uses a collection of org.apache.tomcat.util.descriptor.tld.TldResourcePath.
Internally, Apache Jasper uses String and URI, not URL.
The Set<URL> is entirely on us, not Apache Jasper.
See org.eclipse.jetty.ee10.apache.jsp.JettyTldPreScanned which also does not use Set<URL> too.

jetty-ee9/jetty-ee9-webapp/src/main/java/org/eclipse/jetty/ee9/webapp/MetaInfConfiguration.java

gregw

A few nits, but I can see clearly what it is doing.

...ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/MetaInfConfiguration.java

gregw · 2023-11-15T01:27:49Z

...ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/MetaInfConfiguration.java

+            if (isEmptyFragment(webFrag))
+                return;
+
+            //convert webFragment Resource to one tied to the Context lifecycle


...ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/MetaInfConfiguration.java

jetty-ee9/jetty-ee9-webapp/src/main/java/org/eclipse/jetty/ee9/webapp/MetaInfConfiguration.java

janbartel · 2023-11-16T05:42:17Z

This is about 10% of what PR #10816 does.

This PR doesn't address the other issues with MetaInfConfiguration (that PR #10816 does address)

such as the NPEs present in some of the find methods

the continued use of File / Files / Path APIs

the bad scope of methods

the constant open/close/open/close/etc of Resources for no good reason

the lack of proper use of Resources (note the plural) utility methods

creating Resource objects of jar files themselves, not the contents (there is no reason to do that in this entire class, mount them, lots of code gets simpler! every Resource is a directory!)

I think this PR is a big improvement on what is there - particularly the rigorous lifecycle of the Resources that have mount points.

I would like to merge this, then you can build on top of it for your enhancements.

…e10/webapp/MetaInfConfiguration.java Co-authored-by: Greg Wilkins <gregw@webtide.com>

gregw

All good but one small niggle

gregw · 2023-11-28T22:06:19Z

...ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/MetaInfConfiguration.java

+                for (Resource r : jars)
+                {
+                    Resource dir = r;
+


Suggested change

for (Resource r : jars)

{

Resource dir = r;

for (Resource dir : jars)

{

janbartel added 2 commits November 14, 2023 10:20

Reduce resources kept open by MetaInfConfiguration

473afb5

Use better ResourceFactory regime to reduce references

2aa8f34

janbartel requested review from gregw and joakime November 14, 2023 05:58

janbartel self-assigned this Nov 14, 2023

gregw requested changes Nov 14, 2023

View reviewed changes

...ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/MetaInfConfiguration.java Outdated Show resolved Hide resolved

joakime requested changes Nov 14, 2023

View reviewed changes

Fix

4feecd9

joakime reviewed Nov 14, 2023

View reviewed changes

jetty-ee9/jetty-ee9-webapp/src/main/java/org/eclipse/jetty/ee9/webapp/MetaInfConfiguration.java Outdated Show resolved Hide resolved

Fix

19ed557

janbartel requested review from gregw and joakime November 14, 2023 23:56

gregw previously approved these changes Nov 15, 2023

View reviewed changes

joakime changed the title ~~Jetty 12.0.x reduce resources metainfconfiguration~~ Improve Resource use in MetaInfConfiguration Nov 15, 2023

joakime mentioned this pull request Nov 15, 2023

MetaInfConfiguration scanning of TLDs should follow spec #10899

Closed

Update jetty-ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/e…

1a51854

…e10/webapp/MetaInfConfiguration.java Co-authored-by: Greg Wilkins <gregw@webtide.com>

janbartel dismissed gregw’s stale review via 1a51854 November 27, 2023 22:47

Changes post review

70afc89

janbartel requested a review from gregw November 28, 2023 00:19

gregw previously approved these changes Nov 28, 2023

View reviewed changes

Change post review

c44c319

janbartel dismissed gregw’s stale review via c44c319 November 28, 2023 23:14

gregw approved these changes Nov 29, 2023

View reviewed changes

janbartel merged commit df42cb2 into jetty-12.0.x Nov 29, 2023
7 checks passed

janbartel deleted the jetty-12.0.x-reduce-resources-metainfconfiguration branch November 29, 2023 01:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Resource use in MetaInfConfiguration #10889

Improve Resource use in MetaInfConfiguration #10889

janbartel commented Nov 14, 2023

gregw left a comment

joakime left a comment

joakime Nov 14, 2023

janbartel Nov 14, 2023

joakime Nov 15, 2023

joakime Nov 14, 2023

janbartel Nov 14, 2023

joakime Nov 14, 2023

gregw left a comment

gregw Nov 15, 2023

janbartel commented Nov 16, 2023

gregw left a comment

gregw Nov 28, 2023

Improve Resource use in MetaInfConfiguration #10889

Improve Resource use in MetaInfConfiguration #10889

Conversation

janbartel commented Nov 14, 2023

gregw left a comment

Choose a reason for hiding this comment

joakime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janbartel commented Nov 16, 2023

gregw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment