Load ApplicationContextIntiailizers using the resource loader's class loader

[pantherdd]

When loading the Spring factories, SpringApplication.getSpringFactoriesInstances() uses the thread context classloader instead of the one passed at construction (inside the ResourceLoader). Using Thread.currentThread().getContextClassLoader() has caused an issue in my setup, as I was expecting the passed-in classloader to be used.

If you agree this is a bug, please use getClassLoader() in this method too, like the rest of the class does.

Also, there seem to be a total of 38 places in Spring Boot where this thread context classloader is used. Not sure if those are legitimate uses, but I'd be nice to check them whether they can also use the ResourceLoader instead, so as to avoid further bugs popping up because of the same root cause.

[philwebb]

It feels like a bug to me, but I'd like a second opinion from someone else on the team so I'll flag this one for "team attention".

[wilkinsona]

While the change in this commit doesn't appear to have been intentional, it's been this way since before 1.0 was released. I think we'd need a pretty compelling argument to change it now as it has the potential to be a breaking change.

@pantherdd Can you please describe the issue that you've faced in your setup in as much detail as possible?

[pantherdd]

@wilkinsona I also don't see a reason why this would've been intentional. And even though fixing it might theoretically be a breaking change, to me it seems very likely that in practice, it won't break anything really. The reason is that SpringApplication is usually the entry point into the program, where there are no other classloaders present yet (so all are the same). And if someone tried to use the classloader customization via ResourceLoader constructor arg like I did, they must have run into this issue as well, so fixing it would probably help them too.

Details about my case: I was trying to build up the Spring context inside a custom Ant task, to extract some information about the wiring of the actual production service and document it. The way this works is that Ant creates a new classloader that uses the classpath defined in build.xml for the task, instantiates your task class using that loader, then runs it. The thread context classloader however stays the same as before: the classloader that loaded Ant itself. This original loader doesn't know about Spring (let alone the JARs containing my service classes and config), so META-INF/spring.factories will be completely missing, and SpringApplication will fail.
I could of course simply switch out the thread context classloader while I'm doing Spring stuff inside the Ant task, and replace the original one afterwards, but then what's the point of passing in a ResourceLoader to SpringApplication?

[philwebb]

fixing it might theoretically be a breaking change, to me it seems very likely that in practice

I tend to agree, but since it's possible to work around the issue by setting the context loader directly I think we should only apply a fix to 2.1.x.

[pantherdd]

@philwebb Is there a use-case though, that we can think of, where the current behavior would be helpful? Not saying that I necessarily disagree with only fixing it in a new minor version, but just want to understand what a possible breaking scenario might look like.

[wilkinsona]

Any ApplicationContextInitializer that relies on being loaded by the thread context class loader and that's different to the class loader from the resource loader will be broken. That may well be unlikely, but we have no way of knowing. Someone may have looked at the existing code, not noticed what's possible via a resource loader and relied on the thread context class loader instead. Breaking that scenario in a maintenance release, particularly when there's a workaround, can't be justified. I agree with Phil that if we do anything here, it should be done in 2.1.x at the earliest.

[pantherdd]

Someone may have looked at the existing code, not noticed what's possible via a resource loader and relied on the thread context class loader instead.

@wilkinsona But for this fix to make any difference, they did have to realize what a ResourceLoader is, what it's for, as well as explicitly instantiating one that contains a specific classloader they want Spring Boot to use, and passing it into the constructor. If they didn't, then resourceLoader == null, thus getClassLoader() == Thread.currentThread().getContextClassLoader(), so there's no change in behavior at all.

[wilkinsona]

That really depends on how and why a custom resource loader has been provided. One may have been provided to customise getResource(String) behaviour rather than being particularly interested in the class loader. Any ResourceLoader implementation that doesn't subclass DefaultResourceLoader is quite likely to change the ClassLoader behaviour.

Again, we can't justify making a potentially breaking change in a maintenance release for a problem that can be avoided by setting the thread context class loader.

[pantherdd]

That's fine, as I said, I really just wanted to understand in what scenario would this be a breaking change. But something as simple as a client providing a ResourceLoader only to customize loading of the Spring application banner could be a valid example. If someone were to do that, they would probably implement getClassLoader() as return CustomBannerLoader.class.getClassLoader();, which in theory could be different from the thread's context classloader. Still, it's very unlikely that this would cause an issue, but I understand your point of having to play it totally safe. Let it be a v2.1 fix then. 👍