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

Resource loading fails in Tomcat when Spring Boot is loaded from the common class loader #22119

Closed
ilinas opened this issue Jun 26, 2020 · 5 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@ilinas
Copy link

ilinas commented Jun 26, 2020

Hello there. We have recently attempted to upgrade to Spring Boot 2.3.0 (and then 2.3.1), but unfortunately class loader changes made in #20900 broke our deployment.

We deploy as a skinny WAR (only the application JAR inside WAR) on Tomcat 8.5 where all the libraries are loaded via Tomcats Common class loader (JARs in $CATALINA_HOME/lib). Technically we should be using the Shared class loader, but oh well. We are doing this because we have hundreds of such deployments, so memory usage and disk space is a consideration.

Since #20900 Spring Boot can no longer load resources (like application.properties) if it itself has been loaded by Tomcats Common class loader. Including Spring, Spring Boot, and all other libraries that use Spring annotations inside the WAR file seems to solve the issue, but that is a lot of duplication in our case.

I am not sure if this qualifies as a bug per se, but this is a significant change in behavior, and we are unsure how to best address it?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 26, 2020
@wilkinsona
Copy link
Member

Thanks for the report and sorry for the inconvenience. The change in behaviour that you have seen wasn't anticipated and was unintended.

If I've understood your description of the problem that you are now facing correctly, I think there may have been a bug in 2.2.x as well. I'll try to explain.

Prior to #20900, the statically referenced DefaultResourceLoader would, when it's being instantiated, capture the TCCL at that time. I expect that this would be a webapp-specific ClassLoader. With DefaultResourceLoader loaded by the common class loader, this would then leak the webapp class loader if its webapp was undeployed.

Do you ever have applications a lifecycle that's shorter than that of the Tomcat instance to which they have been deployed?

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Jun 26, 2020
@ilinas
Copy link
Author

ilinas commented Jun 26, 2020

No, we deploy all applications and then start Tomcat. It is also all the same application, just running different configurations.

Wouldn't the actual fix be using the TCCL and not storing it? You really cannot make assumptions about which class loader a particular class is loaded by. Now with 2.3.x you have to make sure that all Spring components and all application components referencing Spring annotations have to be loaded by the same class loader. Which in case of Tomcat gives so many possibilities to get it wrong.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 26, 2020
@ilinas
Copy link
Author

ilinas commented Jun 29, 2020

On a closer look, I think the implementation in 2.2.x was correct. ClassLoader in DefaultResourceLoader is only stored if it is explicitly set via DefaultResourceLoader(@Nullable ClassLoader classLoader) or setClassLoader(@Nullable ClassLoader classLoader). Otherwise ClassLoader getClassLoader() is called which returns ClassUtils.getDefaultClassLoader(). I cannot see how that would cause the ClassLoader to leak.

Or rather I understand the concern, if one sets the ClassLoader explicitly, but from what I can see, the no-parameter new DefaultResourceLoader() is the safe one.

@wilkinsona
Copy link
Member

wilkinsona commented Jun 29, 2020

In Spring Framework 5.2.x (which is the version that Spring Boot 2.2 and 2.3 use), DefaultResourceLoader() captures the ClassLoader at construction time. This creates the potential for a leak. The behaviour of DefaultResourceLoader() was changed in Spring Framework 5.3 to use the TCCL that's in place at the time of resource access.

We'll have to consider making a change like this in Boot. We rejected it previously due to the potential for a larger than wanted change in behaviour, however that didn't consider the situation that you are in.

@ilinas
Copy link
Author

ilinas commented Jun 29, 2020

I see, yes. I was looking at Spring Framework 5.3. Sorry for the confusion.

@philwebb philwebb added type: bug A general bug and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Jun 29, 2020
@philwebb philwebb added this to the 2.3.x milestone Jun 29, 2020
@wilkinsona wilkinsona self-assigned this Jun 30, 2020
@wilkinsona wilkinsona changed the title Class loader changes break in Tomcat with Common class loader Resource loading fails in Tomcat when Spring Boot is loaded from the common class loader Jun 30, 2020
@wilkinsona wilkinsona modified the milestones: 2.3.x, 2.3.2 Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants