Use Tomcat 8's new instrumentable WebappClassLoader [SPR-10788]

[spring-projects-issues]

Nick Williams opened SPR-10788 and commented

First, you can see the companion Tomcat enhancement request at the attached reference URL.

For many application servers, Spring provides a LoadTimeWeaver implementation that can take advantage of the instrumentable ClassLoader that the server provides. Tomcat's WebappClassLoader is currently not instrumentable, so Spring provides a custom ClassLoader that can be used by placing spring-instrument-tomcat.jar in $TOMCAT_HOME/lib and putting a loader declaration in META-INF/context.xml.

It would be nice if load time weaving could "just work" like it does in other application servers. I submitted the attached Tomcat enhancement request and the community seems accepting of the idea of modifying org.apache.catalina.loader.WebappClassLoader to make it instrumentable so that users no longer have to jump through those hoops to get load time weaving working in Tomcat.

I will be submitting a patch to Tomcat in the next week or two to make WebappClassLoader instrumentable. I will then submit a pull request to add org.springframework.instrument.classloading.tomcat.TomcatLoadTimeWeaver (and related classes) to Spring 4.0 (and hopefully also 3.2). The new WebappClassLoader will exist in Tomcat 7.0.43 or 7.0.44 (depending on when my patch is accepted and assuming they agree to back-port it to 7) and Tomcat 8.0.

However, I need the following questions/items addressed by someone here on the Spring development team:

1. I plan on, for the most part, copying-and-pasting the Apache 2.0 licensed code from org.springframework.instrument.classloading.WeavingTransformer and org.springframework.instrument.classloading.tomcat. TomcatInstrumentableClassLoader to the Apache 2.0 licensed Tomcat. Can someone confirm that Spring is willing to donate this code to Tomcat?
2. Some of the Tomcat folks are questioning the need for a getThrowawayClassLoader() method. I'm not quite sure I understand the point of this method, myself. Can someone please clarify its purpose?
3. Once I have completed the Tomcat patch, the Tomcat folks would like someone here to state that it A) looks generally correct, and B) is a useful change. There is someone willing to do this, correct?

While the changes will actually be quite minor, I think this will serve as a huge improvement to using load time weaving in a Tomcat environment.

---

Affects: 3.2 GA, 4.0 M2

Reference URL: https://issues.apache.org/bugzilla/show_bug.cgi?id=55317

Attachments:

• support.log (551.93 kB)

Issue Links:

• Load Time Weaving not working after upgrading Tomcat from 7.0.50 to 7.0.52 [SPR-11446] #16072 Load Time Weaving not working after upgrading Tomcat from 7.0.50 to 7.0.52
• Hibernate session cannot be found after upgrading Tomcat from 7.0.50 to 8.0.3 [SPR-11447] #16073 Hibernate session cannot be found after upgrading Tomcat from 7.0.50 to 8.0.3
• TomcatInstrumentableClassLoader should support Tomcat 7.0.63+ as well [SPR-13210] #17802 TomcatInstrumentableClassLoader should support Tomcat 7.0.63+ as well

[spring-projects-issues]

Juergen Hoeller commented

I definitely agree that this is worth having in Tomcat proper. We tried to donate this before, actually - to Tomcat or our tc Server variant - but that didn't go anywhere. In any case, I'm happy to support the effort now, and have been saying so to the Tomcat guys already. I can review a patch there any time.

Taking our TomcatInstrumentableClassLoader as a starting point is fine since the license is compatible anyway. However, there might be quite a few optimizations possible, not least of it all due to WebappClassLoader changes in Tomcat 7.0/8.0. Our code is based on Tomcat 5.0 and basically had to deal with it as-is.

I don't see a need to have a special TomcatLoadTimeWeaver, actually: As long as the new version of WebappClassLoader follows the conventions that our ReflectiveLoadTimeWeaver expects, just like our TomcatInstrumentableClassLoader does as well, Spring's DefaultContextLoadTimeWeaver can autodetect it via reflection.

A getThrowawayClassLoader() isn't strictly necessary: Our ReflectiveLoadTimeWeaver falls back to a SimpleThrowawayClassLoader if the underlying ClassLoader doesn't declare a getThrowawayClassLoader() method. That method basically only exists for JPA, in order to support PersistenceUnitInfo.getNewTempClassLoader():

http://docs.oracle.com/cd/E17802_01/products/products/persistence/javadoc-1_0-fr/javax/persistence/spi/PersistenceUnitInfo.html#getNewTempClassLoader%28%29

What persistence providers expect there is an independent ClassLoader that is able to temporarily load a few classes without instrumenting them. I'm not a great fan of that approach, but as far as I figure, it was meant as an alternative to doing early introspection on classes via ASM's parsing of class files.

Juergen

[spring-projects-issues]

Juergen Hoeller commented

So from Spring's perspective, this issue is more about tracking Tomcat's effort than about changes on our side. Ideally, our existing ReflectiveLoadTimeWeaver does everything that is needed here already. If there are improvements to do on Spring's side beyond those basics, I'd rather only do them for 4.0.

[spring-projects-issues]

Juergen Hoeller commented

Nick,

I've revised TomcatInstrumentableClassLoader a bit, so it may be a better template for Tomcat's own efforts now and worth another look.

Do you maybe have a few cycles left to give the latest 3.2.4 snapshot a try? This revision here and in particular the persistence.xml parsing changes definitely suggest a round of integration testing. Would be great if it involves JPA weaving on Tomcat, with TomcatInstrumentableClassLoader being set up, and with some exclude-unlisted-classes usage...

Juergen

[spring-projects-issues]

Juergen Hoeller commented

Or alternatively, give the most recent 4.0 snapshot a try - it contains the same changes anyway. Any kind of integration testing would be much appreciated, in particular with the 3.2.4 release scheduled for tomorrow...

If you happen to have the chance to test an application of yours on Tomcat, don't forget to replace the spring-instrument-tomcat.jar in Tomcat's lib directory with the version from the latest Spring snapshot as well.

Juergen

[spring-projects-issues]

Nick Williams commented

Unfortunately, I can't get to this tonight. I will definitely take a look at it tomorrow evening or this weekend, but I'm afraid that'll be too late for 3.2.4.

I did review your <exclude-unlisted-classes> changes and I'm 100% sure that it's right. As for your changes to the weaving classes, I have only a few comments:

1. The JavaDoc still has 4-5 links to http://tomcat.apache.org/tomcat-5.5-doc/. Tomcat 5.5 is archived and no longer supported. Everyone who emails the list about it is told to "upgrade now," and "we can't help you." The links should be changed to Tomcat 6.0.
2. All of the "(for Tomcat 6.x)" should probably say "(for Tomcat 6.x and newer)", since Tomcat 7.x is very mature and Tomcat 8.0 RC1 comes out this weekend (assuming the vote passes, which I suspect it will).
3. getThrowawayClassLoader has the comment, "Use reflection to copy all the fields since most of them are private on pre-5.5 Tomcat." I'm not sure this still applies anymore, for a few reasons:
   • Spring doesn't support Tomcat 5.5 or earlier anymore. Spring requires Servlet 2.5 or newer, IIRC. Tomcat 5.5 is Servlet 2.4, so Spring won't run on it anymore. Therefore, any references to Tomcat 5.5 or earlier could be removed.
   • The necessary fields may all be protected in Tomcat 6.0 now. I'm not sure, but you should review.

---

Now, about this issue specifically. It sounds like the Tomcat community may prefer registerTransformer() and unregisterTranformer() methods instead of addTransformer(). If you can think of any reasons that this is a "bad thing" by all means let me know and I'll pass it on. If not, the WebappClassLoader may not support Spring's addTransformer() convention, and thus would require the aforementioned TomcatLoadTimeWeaver. We'll see.

I'll hopefully have an update on this later next week. Right now I'm focused on getting a patch completed for Java 8. Now, I don't reckon you've had a chance to look at #15390 or (more importantly) #6227 have you? I REALLY want to get those in to RC1, and so I REALLY need to know how I should update my pull requests. :D Thanks!

[spring-projects-issues]

Juergen Hoeller commented

Nick, we're doing the 3.2.4 release on Monday now. So any time for a bit of sanity integration testing up until Monday is alright - and very much appreciated.

I've got a few JPA 2.x related issues for 4.0 RC1 that I intend to resolve today, including #15390.

As for the time zone stuff in #6227, it's a must-have for 4.0 RC1 from my side as well. It's on my work plan for next week, side by side with a JSR-310 review.

Juergen

[spring-projects-issues]

Juergen Hoeller commented

Spring Framework 3.2 actually still supports Servlet 2.4 at runtime, primarily because of WebSphere 6.1 compatibility. So technically, Tomcat 5.5 and even 5.0 should still work - and any kind of server/platform which happens to include a Tomcat 5.x fork.

That said, for Spring Framework 4.0, those references should definitely get updated, since it's all Servlet 2.5+ there - and for quite a few purposes, even Servlet 3.0+. As a consequence, our documentation should explicitly refer to Tomcat 6.0+ there.

The fields in WebappClassLoader are indeed all protected now, but does that help us much? We still need to copy them via reflection if they are protected, and there doesn't seem to be any other support for cloning a WebappClassLoader in Tomcat 6.0 either.

With respect to "addTransformer" versus "registerTransformer": We could update ReflectiveLoadTimeWeaver to check for the existence of either method. It'd be great to avoid a Tomcat compilation dependency, so I'm inclined to hang on to the reflective approach.

Juergen

[spring-projects-issues]

Nick Williams commented

Interesting note about WebSphere 6.1: It's end-of-life was supposed to be September 30, 2012. It was extended for one year. Nevertheless, it's EOL will be before Spring 4.0 releases, so I think it's reasonable to argue that Spring 4.0 can drop support for WebSphere 6.1 (especially if maintaining Servlet 2.4 support is hindering us in some way, but I don't know if that's the case).

Either way, good that we agree that those references should be updated to Tomcat 6.0 since 5.5 is no longer supported. :-)

Since TomcatInstrumentableClassLoader extends WebappClassLoader, then no, you shouldn't need to use reflection to copy the protected fields. With that said, there's nothing inherently wrong with using reflection here; it just isn't the easiest approach.

I like the idea of just updating ReflectiveLoadTimeWeaver to also look for registerTransformer(). I'll remember this when the time comes, but we'll have to wait and see what direction Tomcat takes.

[spring-projects-issues]

Nick Williams commented

(Disregard this comment. It was actually an unrelated bug with Spring Data JPA, which I had started using at the same time as enabling Weaving.)

[spring-projects-issues]

Nick Williams commented

So, I have attached a patch to the Tomcat Bugzilla issue that implements an instrumentable WebappClassLoader in Tomcat. Juergen Hoeller, if you could go review it and give it your approval (if deserved) on the Bugzilla issue or the mailing list, that would be fantastic.

NOTE: After some thorough discussions on the security list, we came to an important conclusion that will affect Spring Framework's support for this feature. The WebappClassLoader lives in org.apache.catalina.loader. If the Tomcat administrator has enabled a SecurityManager, by default it prevents reflective use of all methods in that package. This means that code like the following will fail with a SecurityException in Tomcat:

servletContext.getClassLoader().getClass().getMethod("addTransformer", ClassFileTransformer.class);

In order to allow transformation of web application classes in the presence of a SecurityManager, we added an org.apache.tomcat.InstrumentableClassLoader. With the SecurityManager default policy present, Spring will still be able to access its methods using reflection. The pseudo-code for this is as follows:

if (ReflectionUtils.classExists("org.apache.tomcat.InstrumentableClassLoader")) {
    Class<?> c = ReflectionUtils.getClass("org.apache.tomcat.InstrumentableClassLoader");
    Method addTransformer = c.getMethod("addTransformer");
    Method getThrowawayClassLoader = c.getMethod("copyWithoutTransformers");
    ...
    addTransformer.invoke(servletContext.getClassLoader(), transformer);
}

This works because we are invoking the methods on the accessible InstrumentableClassLoader and passing in an instance of the WebappClassLoader, which implements it.

This means that Spring's current code will technically work, but only for addTransformer and only if no SecurityManager is present. After the Tomcat patch gets accepted, I will submit a pull request to Spring Framework to add a org.springframework.instrument.classloading.tomcat.TomcatLoadTimeWeaver that knows how to handle the InstrumentableClassLoader methods properly.

[spring-projects-issues]

Nick Williams commented

Juergen Hoeller, can you take a look at the patch on the attached Tomcat bugzilla and provide feedback for the Tomcat team? It appears we've gotten the patch to a good place, but they won't commit until someone from Spring says it's useful and looks correct.

Thanks.

[spring-projects-issues]

Juergen Hoeller commented

Since I was in the process of updating our GlassFishLoadTimeWeaver for GlassFish 3+ anyway, I went ahead with a TomcatLoadTimeWeaver at the same time: It's very similar to the current GlassFishLoadTimeWeaver now, capable of handling both Tomcat's new own InstrumentableClassLoader and Spring's old TomcatInstrumentableClassLoader. Our DefaultContextLoadTimeWeaver simply delegates to that TomcatLoadTimeWeaver when encountering an "org.apache.catalina" ClassLoader now, similar to its other detection rules.

Juergen