This started from https://jira.spring.io/browse/SPR-13210

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

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.

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.

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.

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.

*** Bug 58560 has been marked as a duplicate of this bug. ***

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.

Hi,

This is fixed for 8.0.33 onwards.

Regards,
Violeta

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

Created attachment 35700 [details]
Suggested patch for TC 7

Minimal patch for TC 7

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.

The example webapp is available at

https://gist.github.com/rainerjung/99802d9cf321b9c594665b2933d8ea49

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.

This was fixed in r1823460