Set classloader for JMX endpoints to application classloader

[Dav1dde]

Related to and fixes: #12088

Sets the Thread's classloader for JMX endpoints, before invoking the endpoint, to the classloader which was initially used to load the context.

Before:

--- Class Loaders
getClass().getClassLoader(): java.net.URLClassLoader@4ac20dd2
currentThread().getContextClassLoader(): sun.misc.Launcher$AppClassLoader@18b4aac2
ClassUtils.getDefaultClassLoader(): sun.misc.Launcher$AppClassLoader@18b4aac2
---

After:

--- Class Loaders
getClass().getClassLoader(): java.net.URLClassLoader@4ac20dd2
currentThread().getContextClassLoader(): java.net.URLClassLoader@4ac20dd2
ClassUtils.getDefaultClassLoader(): java.net.URLClassLoader@4ac20dd2
---

Test-Code (using sample Application from start.spring.io including web and actuator):

@Component
@JmxEndpoint(id = "classloader")
public class JmxClassloaderEndpoint {
    @WriteOperation
    public void someOperation() {
        System.out.println("--- Class Loaders");
        System.out.println("getClass().getClassLoader(): " + getClass().getClassLoader());
        System.out.println("currentThread().getContextClassLoader(): " + Thread.currentThread().getContextClassLoader());
        System.out.println("ClassUtils.getDefaultClassLoader(): " + ClassUtils.getDefaultClassLoader());
        System.out.println("---");
    }
}

[Dav1dde]

Build failure seems to be random and not related to this change, can someone re-trigger the build?

[wilkinsona]

Closing and re-opening to trigger a new build.

[wilkinsona]

I think that used to work with Travis. Doesn’t appear to have worked with Concourse.

[wilkinsona]

I’ve manually triggered a new build on Concourse but I’m not sure it’s building the right change. There are some git-related errors in the logs.

[Dav1dde]

Any news or interest in this?

[snicoll]

@Dav1dde there is. I had a look at your proposal a while back and didn't like it very much but I have nothing else to offer. I'll have to dig a bit more first.

[Dav1dde]

@snicoll thank you!

You're right, it feels hack-ish. Want me to update it so the classloader comes from BeanClassLoaderAware instead of getClass().getClassLoader()?

[snicoll]

That won't change much I am afraid. Switching the classloader of the current thread without restoring the previous instance once the method has been invoked sounds definitely wrong to me. I would only do this as a last resort option.

[Dav1dde]

Oh, that's not a problem, actually had a version that restored it. (that's also what we do at work right now). I'll update this tonight and let's see if you like that more.

[snicoll]

Sorry, I meant that this proposal doesn’t look great to me (and at the very least the classloader should be restored).

[Dav1dde]

Updated and restoring class loader now (no-op if bean class loader is not set, e.g. in tests). I personally don't like the solution either but I can't think of anything better, if you can think of something let me know and I can change the PR accordingly.

[snicoll]

Thanks @Dav1dde, that's a very nice summary of the situation indeed. I'll spend a bit more time on it and report here when I am done.

[wilkinsona]

This may throw a SecurityException. I think we should set the TCCL on a best-effort basis so any SecurityException that's thrown should be caught (and logged) and we should then proceed with the invoke.

[wilkinsona]

FWIW, other than the SecurityException that I've just commented about, I think we should make this change.

[Dav1dde]

I'll update it later tonight.

[Dav1dde]

I considered having the class loader as a constructor argument, but for me the class loader is optional for the EndpointMBean.

[Dav1dde]

Not sure if a info log would be better here.

[Dav1dde]

Branch updated, I added a few comments where I am not sure if things are okay this way and fit into the spring codebase.

[snicoll]

@Dav1dde thank you for your first contribution to Spring Boot. This has been merged in 2.0.x and master.