TomcatEmbeddedWebappClassLoader.getResources() returns 2 entries for a single resource in a fat war

[jerrinot]

I have a Spring Boot 1.5.3 application, packaged as an executable WAR with embedded Tomcat.
When I call tccl.getResources() then I am getting 2 entries for a single physical resource.

Here is a reproducer: https://github.com/jerrinot/booottest

When I build it & execute it:

mvn clean install
java -jar target/boottest-0.0.1-SNAPSHOT.war

and access http://localhost:8080/ then I see this in the sysout:

jar:file:/home/jara/devel/tmp/github/boottest/target/boottest-0.0.1-SNAPSHOT.war!/WEB-INF/classes!/myResource
war:file:/home/jara/devel/tmp/github/boottest/target/boottest-0.0.1-SNAPSHOT.war*/WEB-INF/classes/myResource

This appears as a bug - a single physical resource should not produce 2 entries with different URLs.

It's working fine when I change the packaging from WAR to JAR.

EDIT: Obviously the affected Spring Boot version is 1.5.3, not 1.5.6 :)

[philwebb]

@wilkinsona Could this be related to the issues in #8299?

[wilkinsona]

I don't think so, but I also wouldn't rule it out. Another possibility is the recent changes to Tomcat's resource URLs: the */ separator is new and the fact that it's a war: URL may be too. I need to dig a bit to figure out what's causing this.

[wilkinsona]

The problem also occurs with Tomcat 8.5.4 which does not use the */ separator:

jar:file:/Users/awilkinson/dev/temp/booottest/target/boottest-0.0.1-SNAPSHOT.war!/WEB-INF/classes!/myResource
jar:file:/Users/awilkinson/dev/temp/booottest/target/boottest-0.0.1-SNAPSHOT.war!/WEB-INF/classes/myResource

And with Spring Boot 1.5.1 which does not contain the changes for #8299:

jar:file:/Users/awilkinson/dev/temp/booottest/target/boottest-0.0.1-SNAPSHOT.war!/WEB-INF/classes!/myResource
war:file:/Users/awilkinson/dev/temp/booottest/target/boottest-0.0.1-SNAPSHOT.war*/WEB-INF/classes/myResource

Lastly, 1.4.0 has the same problem:

jar:file:/Users/awilkinson/dev/temp/booottest/target/boottest-0.0.1-SNAPSHOT.war!/WEB-INF/classes!/myResource
jar:file:/Users/awilkinson/dev/temp/booottest/target/boottest-0.0.1-SNAPSHOT.war!/WEB-INF/classes/myResource

In short, things have been this way for a while and, the different separator and URL scheme aside, haven't changed recently

[wilkinsona]

This is a variant of the problem described in #7449 and fixed in 5e79657.

The first of the three scenarios that I listed above still occurs as, unlike Spring Framework's PathMatchingResourcePatternResolver, the reproducer does not consider the equality of the URLs. If avoiding duplicates is important, that's a bug in the reproducer as it's making assumptions about the class path and class loader structure that may not hold true.

The approach taken in 5e79657 won't work here as it's a jar: URL and a war: URL that are being compared. We only have a handler for the former and, therefore, only have control over half of the problem.

[jerrinot]

@wilkinsona: thanks for analysis. I am trying to understand it. What do you think the reproducer should do to detect duplicates? I still find it strange that a single physical resources results in multiple entries. This is highly unexpected.

[wilkinsona]

What do you think the reproducer should do to detect duplicates?

It should compare their equality. One easy way to do so (and this is what Spring Framework'sPathMatchingResourcePatternResolver does) is to place them in a Set. PathMatchingResourcePatterResolver preserves ordering by using a LinkedHashSet.

I still find it strange that a single physical resources results in multiple entries. This is highly unexpected.

You need to expect it as, unless you have complete control over all of the ClassLoaders involved, there's no guarantee that the same physical resource won't be visible to more than one ClassLoader in the hierarchy. Here's a (slightly contrived) example:

File foo = new File("foo");
foo.mkdirs();
File bar = new File(foo, "bar.txt");
bar.createNewFile();

URLClassLoader parent = new URLClassLoader(new URL[] {foo.toURI().toURL()}, null);
URLClassLoader child = new URLClassLoader(new URL[] {foo.toURI().toURL()}, parent);

Enumeration<URL> resources = child.getResources("bar.txt");
while(resources.hasMoreElements()) {
	System.out.println(resources.nextElement());
}

The resulting enumeration contains two entries, both for the exact same URL because it's on the class path of both class loaders in the hierarchy.

[tombujok]

@snicoll Would be great if you could fix this team. We have a couple of users complaining about it:
hazelcast/hazelcast#10438
Thx in advance!

[wilkinsona]

@tombujok Unfortunately, there's no obvious solution so it may be a while before we have a fix. I believe it will only affect users building a war and executing with java -jar. Unless those users are using JSPs, they would be better building an executable jar instead.

[wilkinsona]

Here's a possible fix:

diff --git a/spring-boot/src/main/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedWebappClassLoader.java b/spring-boot/src/main/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedWebappClassLoader.java
index a2b156001c..b3e72c0f14 100644
--- a/spring-boot/src/main/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedWebappClassLoader.java
+++ b/spring-boot/src/main/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedWebappClassLoader.java
@@ -16,7 +16,10 @@

 package org.springframework.boot.context.embedded.tomcat;

+import java.io.IOException;
 import java.net.URL;
+import java.util.Collections;
+import java.util.Enumeration;

 import org.apache.catalina.loader.WebappClassLoader;
 import org.apache.commons.logging.Log;
@@ -24,7 +27,7 @@ import org.apache.commons.logging.LogFactory;

 /**
  * Extension of Tomcat's {@link WebappClassLoader} that does not consider the
- * {@link ClassLoader#getSystemClassLoader() system classloader}. This is required to to
+ * {@link ClassLoader#getSystemClassLoader() system classloader}. This is required to
  * ensure that any custom context classloader is always used (as is the case with some
  * executable archives).
  *
@@ -44,6 +47,16 @@ public class TomcatEmbeddedWebappClassLoader extends WebappClassLoader {
        }

        @Override
+       public URL findResource(String name) {
+               return null;
+       }
+
+       @Override
+       public Enumeration<URL> findResources(String name) throws IOException {
+               return Collections.emptyEnumeration();
+       }
+
+       @Override
        public synchronized Class<?> loadClass(String name, boolean resolve)
                        throws ClassNotFoundException {
                Class<?> result = findExistingLoadedClass(name);

The theory behind the fix is that all WebappClassLoader does in findResource(String) and findResources(String) is look in WEB-INF/classes. Unlike standalone Tomcat, we know that in a Boot app WEB-INF/classes is on the classpath of WebappClassLoader's parent so the resources will be found when the parent is queried (which happens as part of the normal search algortithm for both getResource(String) and getResources(String)).

With the change in place, the reproducer behaves as desired when run with java -jar, mvn spring-boot:run and in an IDE. It also has the benefit of bypassing the problem described in #9750.

This feels like a pretty big change to be making in 1.5.x. @markt-asf Is what I'm proposing completely daft? Is there anything that I've overlooked that we'd lose by making findResource(String) and findResources(String) return nothing and just relying on the parent?

[markt-asf]

It does strike me as odd that /WEB-INF/classes is on the class path for the parent of the web application class loader. I assume there is something I'm missing that would explain this.
The proposal doesn't look daft to me. I don't see anything that jumps out at me as being problematic but it is the sort of change that makes me worry about regressions for some edge cases (although I can't think of any right now).

[wilkinsona]

Thanks, @markt-asf.

It does strike me as odd that /WEB-INF/classes is on the class path for the parent of the web application class loader. I assume there is something I'm missing that would explain this.

When you launch a war file with java -jar, Boot creates a LaunchedURLClassLoader with the contents of WEB-INF/lib and WEB-INF/classes on its class path. This allows the application's classes and its dependencies to be loaded. This is required for the initial bootstrapping of the application.

During start up of the application, Boot detects the need for an embedded servlet container and that Tomcat should be used. At this point, it creates an embedded Tomcat instance and configures a Context with TomcatEmbeddedWebappClassLoader as its loader class and LaunchedURLClassLoader is used as the parent.

The proposal doesn't look daft to me

That's reassuring. Thank you.

I don't see anything that jumps out at me as being problematic but it is the sort of change that makes me worry about regressions for some edge cases

It makes me worry too. I think we may be best making this change in a 2.0 milestone and seeing how it goes. If it works as hoped we can then consider backporting it to 1.5.x.

[javaConductor]

Was this fix backported to 1.5.* ? I see the fix merged into v2.0.3.RELEASE only.

[wilkinsona]

No. We considered it to be too risky to include in 1.5. The fix is in 2.0 M3 and later.

[javaConductor]

Is there any workaround for 1.5.X that I can use ? I have hit this bug but I am unable to upgrade to 2.0.X yet.

[wilkinsona]

Everything we know about the problem is in this issue. The best workaround that we know of is to use jar packaging. If that’s not an option because you are using JSPs, you could switch to Jetty.

[patrickconant]

We've worked around this issue by including spring-boot 2.x class org.springframework.boot.context.embedded.tomcat.TomcatEmbeddedWebappClassLoader in our spring-boot apps. It's an ugly workaround but it works until we can get to spring-boot 2.x and we haven't seen any adverse effects.

[philwebb]

@patrickconant I'm glad that works for you, but it's not an approach I'd generally recommend people adopt.

[patrickconant]

I understand. It's risky and sub-optimal (like anything labelled a "workaround"), and I'm certainly not offering to support the resulting miscreation. I just wanted to provide it as an option to anyone that stumbles across this problem who cannot upgrade to 2.x for whatever reason.