Bug 58143 - The WebppClassLoader doesn't call transformers on cached classes
Summary: The WebppClassLoader doesn't call transformers on cached classes
Status: REOPENED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 8.0.24
Hardware: PC All
: P3 enhancement (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
: 58560 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-07-15 16:55 UTC by Andrei Ivanov
Modified: 2018-01-28 10:09 UTC (History)
3 users (show)



Attachments
Suggested patch for TC 7 (2.89 KB, patch)
2018-01-28 09:43 UTC, Rainer Jung
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrei Ivanov 2015-07-15 16:55:31 UTC
The Spring Framework load time weaving mechanism registers class file transformers when the web app is deployed:

WebappClassLoader(WebappClassLoaderBase).addTransformer(ClassFileTransformer) line: 666
	TomcatLoadTimeWeaver.addTransformer(ClassFileTransformer) line: 88	
	DefaultContextLoadTimeWeaver.addTransformer(ClassFileTransformer) line: 143	
	AspectJWeavingEnabler.enableAspectJWeaving(LoadTimeWeaver, ClassLoader) line: 83	
	AspectJWeavingEnabler.postProcessBeanFactory(ConfigurableListableBeanFactory) line: 71	
	PostProcessorRegistrationDelegate.invokeBeanFactoryPostProcessors(Collection<BeanFactoryPostProcessor>, ConfigurableListableBeanFactory) line: 284	
	PostProcessorRegistrationDelegate.invokeBeanFactoryPostProcessors(ConfigurableListableBeanFactory, List<BeanFactoryPostProcessor>) line: 174	
	XmlWebApplicationContext(AbstractApplicationContext).invokeBeanFactoryPostProcessors(ConfigurableListableBeanFactory) line: 658	
	XmlWebApplicationContext(AbstractApplicationContext).refresh() line: 504	
	ContextLoaderListener(ContextLoader).configureAndRefreshWebApplicationContext(ConfigurableWebApplicationContext, ServletContext) line: 446	
	ContextLoaderListener(ContextLoader).initWebApplicationContext(ServletContext) line: 328	
	ContextLoaderListener.contextInitialized(ServletContextEvent) line: 107	
	StandardContext.listenerStart() line: 4729	
	StandardContext.startInternal() line: 5167	
	StandardContext(LifecycleBase).start() line: 150	
	StandardHost(ContainerBase).addChildInternal(Container) line: 725	
	StandardHost(ContainerBase).addChild(Container) line: 701	
	StandardHost.addChild(Container) line: 717	
	HostConfig.deployWAR(ContextName, File) line: 945	
	HostConfig$DeployWar.run() line: 1768	
	Executors$RunnableAdapter<T>.call() line: 511	
	FutureTask<V>.run() line: 266	
	ThreadPoolExecutor.runWorker(ThreadPoolExecutor$Worker) line: 1142	
	ThreadPoolExecutor$Worker.run() line: 617	
	Thread.run() line: 745	

After this moment, any classes loaded will be transformed, if necessary.

My problem is that Tomcat loads some classes before these transformers are registered:
	WebappClassLoader(WebappClassLoaderBase).findResourceInternal(String, String, boolean) line: 2639	
	WebappClassLoader(WebappClassLoaderBase).findResource(String) line: 936	
	WebappClassLoader(WebappClassLoaderBase).getResourceAsStream(String) line: 1115	
	ContextConfig.populateJavaClassCache(String) line: 2165	
	ContextConfig.populateJavaClassCache(String, JavaClass) line: 2155	
	ContextConfig.checkHandlesTypes(JavaClass) line: 2060	
	ContextConfig.processAnnotationsStream(InputStream, WebXml, boolean) line: 2012	
	ContextConfig.processAnnotationsJar(URL, WebXml, boolean) line: 1961	
	ContextConfig.processAnnotationsUrl(URL, WebXml, boolean) line: 1936	
	ContextConfig.processAnnotations(Set<WebXml>, boolean) line: 1897	
	ContextConfig.webConfig() line: 1149	
	ContextConfig.configureStart() line: 771	
	ContextConfig.lifecycleEvent(LifecycleEvent) line: 305	
	LifecycleSupport.fireLifecycleEvent(String, Object) line: 117	
	StandardContext(LifecycleBase).fireLifecycleEvent(String, Object) line: 90	
	StandardContext.startInternal() line: 5066	
	StandardContext(LifecycleBase).start() line: 150	
	StandardHost(ContainerBase).addChildInternal(Container) line: 725	
	StandardHost(ContainerBase).addChild(Container) line: 701	
	StandardHost.addChild(Container) line: 717	
	HostConfig.deployWAR(ContextName, File) line: 945	
	HostConfig$DeployWar.run() line: 1768	
	Executors$RunnableAdapter<T>.call() line: 511	
	FutureTask<V>.run() line: 266	
	ThreadPoolExecutor.runWorker(ThreadPoolExecutor$Worker) line: 1142	
	ThreadPoolExecutor$Worker.run() line: 617	
	Thread.run() line: 745	

This makes subsequent calls to WebappClassLoaderBase.findResourceInternal to return cached resources:
ResourceEntry entry = resourceEntries.get(path);
if (entry != null) {
    return entry;
}

These cached resources will not go through the transformers, which are called bellow.

Maybe the resourceEntries cache could be flushed somehow?
Comment 1 Andrei Ivanov 2015-07-15 16:56:02 UTC
This started from https://jira.spring.io/browse/SPR-13210
Comment 2 Violeta Georgieva 2015-07-20 14:44:53 UTC
Hi,

Here are my comments:

The most obvious solution here is to clear WebappClassLoaderBase.resourceEntries when a transformer is added but I have some concerns about it.

When you are using TomcatInstrumentableClassLoader it is defined in context.xml and this class loader will be used to load the classes from the very beginning (please correct me if this is not the case from spring point of view)

When you are using TomcatLoadTimeWeaver (this bug) the transformer is added on contextInitialed phase which, my opinion, is too late.
I think that this (TomcatLoadTimeWeaver) should be added on an earlier stage (e.g. the loader created event or configure_start event). Otherwise:
- ServletContainerInitializers will work with non-transformed classes
- Annotations scanning will happen on non-transformed classes
- Depending on ServletContextListeners order, the listeners before Spring's one will work with non-transformed classes

Why I think that clearing resourceEntries is not a good solution:
- There might appear ClassCastExceptions between components using non-transformed and transformed classes.

Other solution will be to mark the loader as modified when a transformer is added so that the context will be reloaded (if it supports reloadable).
This won't solve the Spring's case because they will add the transformer again on the contextInitialized step and we will enter in never ending cycle.

Other opinions?

Regards,
Violeta
Comment 3 Mark Thomas 2015-07-30 14:41:08 UTC
I don't think we can fix this.

The TomcatLoadTimeWeaver could be configured slightly earlier (in an SCI) but that is the earliest specification defined hook that could be used. This still isn't early enough to catch everything since any classes defined in a HandlesType for an SCI will already have been loaded by this point.

Anything earlier requires Tomcat specific configuration at which point you might as well use the TomcatInstrumentableClassLoader and configure it in a META-INF/context.xml.

I'll bring this to the attention of the Spring folks via $dayjob to see if they can come up with anything better.
Comment 4 Mark Thomas 2015-09-01 12:30:49 UTC
I had some discussions with the Spring folks at $dayjob and they haven't been able to come up with a better solution either.

The only other option that did come up in the discussion was avoiding the annotation and HandlesType scanning. It should be possible to avoid the annotation scanning by explicit definitions in web.xml. The HandlesType scanning can be disabled but there is no alternative available to replace it.

You may ned up having to use TomcatInstrumentableClassLoader.
Comment 5 Andrei Ivanov 2015-09-01 12:36:09 UTC
Thanks :-)
I've managed to activate the TomcatInstrumentableClassLoader, as the context.xml in the WAR was ignored when deploying the app in a virtual host with the context defined there.
Comment 6 Bryn 2015-09-04 22:15:22 UTC
As suggested by Mark I have reopened this as an enhancement.

I have also come across this issue. In short registering a transformer in a ServletContextInitializer was too late for classes that are scanned as part of the @HandlesTypes mechanism, they are already loaded and are therefore not transformed.

It would be great if we could have an alternative mechanism to register transformers before scanning takes place.

Ideally this would be automatic registration rather than requiring the developer to configure anything, thus allowing them to include the library with the transformer in and benefit without referring to installation instructions. This was my original goal of registering the transformer in a ServletContextInitializer.

With this in mind the ServiceLoader mechanism or something similar could be considered.

Library developers would create a resource containing the fully qualified class names of their transformers at:
META-INF/services/java.lang.instrument.ClassFileTransformer

Transformers would be enumerated and registered before scanning takes place.
 
Even if JDK ServiceLoader implementation is not used, perhaps due to performance, then the mechanism is at least well understood by developers. Instead of having custom config specific to Tomcat the docs can just say that ClassFileTransformers are discovered via ServiceLoader. This raises the chances that other servlet containers would adopt the same approach. 

I note that Jetty have an addTransformer method on their classloader as well, but using ServiceLoader would be container independent.
Comment 7 Mark Thomas 2015-10-28 16:28:58 UTC
*** Bug 58560 has been marked as a duplicate of this bug. ***
Comment 8 Andrei Ivanov 2016-04-08 07:52:23 UTC
I think this can be closed now that since 8.0.33 the class cache is gone, rendering TomcatInstrumentableClassLoader useless, but making the TomcatLoadTimeWeaver work.
Comment 9 Violeta Georgieva 2016-04-11 07:15:01 UTC
Hi,

This is fixed for 8.0.33 onwards.

Regards,
Violeta
Comment 10 Rainer Jung 2018-01-28 09:39:25 UTC
Reopening this for TC 7.

My situation:

- not using Springs TomcatInstrumentableClassLoader  but instead relying on the normal Tomcat WebappClassLoader (and WebappClassLoaderBase) implementing InstrumentableClassLoader

- using Spring load time weaving

That combination works starting with the implementation of InstrumentableClassLoader in 7.0.64 but is broken again since 7.0.70.

Reason is the reorganization of the resourceEntries cache. During context initialization the class which should get woven is loaded two times. Once as a resource (and at a time when the weaver was not yet added to the class loader) and then again as a class with the weaver in place. Starting with 7.0.70 the loading as a resource and as a class use the same key in the resourceEntries cache. Since the weaving does not happen when a class is served from the cache, this breaks it.

In TC 8, 8.5 and 9 there was a later change in WebappClassLoaderBase which makes it work again. The weaving was moved from close to the place were resources get added to resourceEntries in findResourceInternal() to findClassInternal().

That change also does quite a few other things, so I isolated it moving the weaver call and tested it in TC 7 with the unit tests. This did not show any problems.

Originally the problem was observed using Spring 3.0, but the behavior is unchanged for at least of Spring 3.0-4.3.9.

I will attach a suggested patch and also a simple example webapp named "weave". Calling the URI /weave/ will respond with "Hello World!" if weaving succeeds, and with "Unwoven" if not. See the trivial class Greeting, the beans file weave.xml, its declaration in web.xml and the index.jsp, all of which are very small. Unfortunately the war file is 6 MB due to the size of the included Spring jar files.

Regards,

Rainer
Comment 11 Rainer Jung 2018-01-28 09:43:24 UTC
Created attachment 35700 [details]
Suggested patch for TC 7

Minimal patch for TC 7
Comment 12 Rainer Jung 2018-01-28 09:47:19 UTC
The fix for trunk, which was backported to 8.5 and 8 but not to seven was:

r1730946 | markt | 2016-02-17 22:16:40 +0100 (Wed, 17 Feb 2016) | 1 line

Refactor class loading so JAR scanning does not trigger the caching of the byte[] for every scanned class until the class is loaded.
Comment 13 Rainer Jung 2018-01-28 09:57:12 UTC
The example webapp is available at

https://gist.github.com/rainerjung/99802d9cf321b9c594665b2933d8ea49
Comment 14 Rainer Jung 2018-01-28 10:09:26 UTC
Finally for the sake of completeness the stacks of the two loading calls for the class to get woven.

First call (happening before Spring called addTransformer()). Here name="de/kippdata/demo/weave/Greeting.class", path="/de/kippdata/demo/weave/Greeting.class", so the class was loaded as a resource:

        at org.apache.catalina.loader.WebappClassLoaderBase.findResourceInternal(WebappClassLoaderBase.java:3194)
        at org.apache.catalina.loader.WebappClassLoaderBase.findResource(WebappClassLoaderBase.java:1473)
        at org.apache.catalina.loader.WebappClassLoaderBase.getResourceAsStream(WebappClassLoaderBase.java:1701)
        at org.springframework.core.OverridingClassLoader.openStreamForClass(OverridingClassLoader.java:166)
        at org.springframework.core.OverridingClassLoader.loadBytesForClass(OverridingClassLoader.java:142)
        at org.springframework.context.support.ContextTypeMatchClassLoader$ContextOverridingClassLoader.loadClassForOverriding(ContextTypeMatchClassLoader.java:111)
        at org.springframework.core.OverridingClassLoader.loadClass(OverridingClassLoader.java:90)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:247)
        at org.springframework.core.OverridingClassLoader.loadClass(OverridingClassLoader.java:84)
        at org.springframework.context.support.ContextTypeMatchClassLoader.loadClass(ContextTypeMatchClassLoader.java:72)
        at org.springframework.util.ClassUtils.forName(ClassUtils.java:250)
        at org.springframework.beans.factory.support.AbstractBeanFactory.doResolveBeanClass(AbstractBeanFactory.java:1429)
        at org.springframework.beans.factory.support.AbstractBeanFactory.resolveBeanClass(AbstractBeanFactory.java:1377)
        at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.determineTargetType(AbstractAutowireCapableBeanFactory.java:641)
        at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.predictBeanType(AbstractAutowireCapableBeanFactory.java:609)
        at org.springframework.beans.factory.support.AbstractBeanFactory.isFactoryBean(AbstractBeanFactory.java:1484)
        at org.springframework.beans.factory.support.DefaultListableBeanFactory.doGetBeanNamesForType(DefaultListableBeanFactory.java:425)
        at org.springframework.beans.factory.support.DefaultListableBeanFactory.getBeanNamesForType(DefaultListableBeanFactory.java:395)
        at org.springframework.context.support.PostProcessorRegistrationDelegate.invokeBeanFactoryPostProcessors(PostProcessorRegistrationDelegate.java:81)
        at org.springframework.context.support.AbstractApplicationContext.invokeBeanFactoryPostProcessors(AbstractApplicationContext.java:687)
        at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:525)
        at org.springframework.web.context.ContextLoader.configureAndRefreshWebApplicationContext(ContextLoader.java:443)
        at org.springframework.web.context.ContextLoader.initWebApplicationContext(ContextLoader.java:325)
        at org.springframework.web.context.ContextLoaderListener.contextInitialized(ContextLoaderListener.java:107)
        at org.apache.catalina.core.StandardContext.listenerStart(StandardContext.java:5109)
        at org.apache.catalina.core.StandardContext.startInternal(StandardContext.java:5632)
        at org.apache.catalina.util.LifecycleBase.start(LifecycleBase.java:145)
        at org.apache.catalina.core.ContainerBase.addChildInternal(ContainerBase.java:1015)
        at org.apache.catalina.core.ContainerBase.addChild(ContainerBase.java:991)
        at org.apache.catalina.core.StandardHost.addChild(StandardHost.java:652)
        at org.apache.catalina.startup.HostConfig.deployWAR(HostConfig.java:1127)
        at org.apache.catalina.startup.HostConfig$DeployWar.run(HostConfig.java:2020)
        at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:439)
        at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:303)
        at java.util.concurrent.FutureTask.run(FutureTask.java:138)
        at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:895)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:918)
        at java.lang.Thread.run(Thread.java:662)


Second call, now with transformers in place but with class already cached. Here name="de.kippdata.demo.weave.Greeting", path="/de/kippdata/demo/weave/Greeting.class", so the class was loaded as an actual class:

        at org.apache.catalina.loader.WebappClassLoaderBase.findResourceInternal(WebappClassLoaderBase.java:3194)
        at org.apache.catalina.loader.WebappClassLoaderBase.findClassInternal(WebappClassLoaderBase.java:3060)
        at org.apache.catalina.loader.WebappClassLoaderBase.findClass(WebappClassLoaderBase.java:1388)
        at org.apache.catalina.loader.WebappClassLoaderBase.loadClass(WebappClassLoaderBase.java:1876)
        at org.apache.catalina.loader.WebappClassLoaderBase.loadClass(WebappClassLoaderBase.java:1750)
        at org.springframework.util.ClassUtils.forName(ClassUtils.java:250)
        at org.springframework.beans.factory.support.AbstractBeanDefinition.resolveBeanClass(AbstractBeanDefinition.java:401)
        at org.springframework.beans.factory.support.AbstractBeanFactory.doResolveBeanClass(AbstractBeanFactory.java:1432)
        at org.springframework.beans.factory.support.AbstractBeanFactory.resolveBeanClass(AbstractBeanFactory.java:1377)
        at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.determineTargetType(AbstractAutowireCapableBeanFactory.java:641)
        at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.predictBeanType(AbstractAutowireCapableBeanFactory.java:609)
        at org.springframework.beans.factory.support.AbstractBeanFactory.isFactoryBean(AbstractBeanFactory.java:1484)
        at org.springframework.beans.factory.support.DefaultListableBeanFactory.doGetBeanNamesForType(DefaultListableBeanFactory.java:425)
        at org.springframework.beans.factory.support.DefaultListableBeanFactory.getBeanNamesForType(DefaultListableBeanFactory.java:395)
        at org.springframework.context.support.DefaultLifecycleProcessor.getLifecycleBeans(DefaultLifecycleProcessor.java:275)
        at org.springframework.context.support.DefaultLifecycleProcessor.startBeans(DefaultLifecycleProcessor.java:133)
        at org.springframework.context.support.DefaultLifecycleProcessor.onRefresh(DefaultLifecycleProcessor.java:114)
        at org.springframework.context.support.AbstractApplicationContext.finishRefresh(AbstractApplicationContext.java:880)
        at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:546)
        at org.springframework.web.context.ContextLoader.configureAndRefreshWebApplicationContext(ContextLoader.java:443)
        at org.springframework.web.context.ContextLoader.initWebApplicationContext(ContextLoader.java:325)
        at org.springframework.web.context.ContextLoaderListener.contextInitialized(ContextLoaderListener.java:107)
        at org.apache.catalina.core.StandardContext.listenerStart(StandardContext.java:5109)
        at org.apache.catalina.core.StandardContext.startInternal(StandardContext.java:5632)
        at org.apache.catalina.util.LifecycleBase.start(LifecycleBase.java:145)
        at org.apache.catalina.core.ContainerBase.addChildInternal(ContainerBase.java:1015)
        at org.apache.catalina.core.ContainerBase.addChild(ContainerBase.java:991)
        at org.apache.catalina.core.StandardHost.addChild(StandardHost.java:652)
        at org.apache.catalina.startup.HostConfig.deployWAR(HostConfig.java:1127)
        at org.apache.catalina.startup.HostConfig$DeployWar.run(HostConfig.java:2020)
        at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:439)
        at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:303)
        at java.util.concurrent.FutureTask.run(FutureTask.java:138)
        at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:895)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:918)
        at java.lang.Thread.run(Thread.java:662)


Before 7.0.70, the two calls did not use the same path for resource and class, so they did not share the cache entry. After the optimization in r1745608 they do.