I have no objections in principle but obviously I'd want to see the proposed patch first.

I will say that someone from the Spring project will need to confirm that any proposed patch is a) correct and b) helpful.

Why would one need to extend the webapp class loader? A quick look to java.lang.instrument indicates that one should be able to do it with a normal instrumentation agent. Can you clarify?

The registered ClassFileTransformers will get the classloader passed in in their transform() call so can decide whether it is a webapp classloader and it wants to act on the class.

Probably it would be nice if there were an easy way to associate the webapp with the WebappClassLoader, or at least the WebappLoader could be retrieved from the WebappClassLoader. From there one can get the necessary info about the webapp via the container.

Sorry, me again: if we want to support it in an agent free way, it seems it woul suffice to add a addTransformer() (or probably better registerTransformer() and unregisterTranformer()) call to the WebappClassLoader that maintains a list of added tranformers and calls them in findResourceInternal() similar to the original spring code. The original spring code calls all of them in turn until the first one succeeds. Don't know whether it would be better to proceed calling in registration order to allow mutliple transformations to occur.

There's no real need to support the proprietry getThrowawayClassLoader() method, which makes most of the lines unnecessary and we'll also not need the WeavingTransformer because we can call the registered Transformers directly.

As far as I can see spring can support the getThrowawayClassLoader() on top of that without subclassing our class loader.

WDYT?

First, here's the companion Spring Framework enhancement request: https://jira.springsource.org/browse/SPR-10788

/I/ don't need to extend WebappClassLoader. I plan on simply adding to the existing WebappClassLoader. Spring Framework extends it because that's the only way to make it work without changing Tomcat directly. The reason any changes are needed at all is because a standard ClassLoader does not just support instrumentation out-of-the-box. You must instrument a class file /as it is being loaded/, before it is initialized/instantiated.

I like the idea of creating registerTransformer() and unregisterTranformer() instead of addTransformer(), but that is a deviation from the unofficial convention that other containers abide by. If Mark is also okay with registerTransformer() and unregisterTranformer(), I'll take that approach.

I'm not exactly sure what getThrowawayClassLoader() is for, either. I have asked some Spring folks to clarify. It exists consistently across containers and Spring container-support classes, so I assume it must be important. We'll see.

I have reservations around the security consequences of providing anything that has access to the webapp classloader the ability to modify any code defined by that classloader. 

Currently, that has to be specifically enabled by adding an agent to the JVM or by adding special classloader to the container installation and enabling it in a web application's Context. This change would automatically enable this for all applications. Further, if an application was enabled for cross-context dispatch it would also be able to register a transformer to modify the code of other applications.

Some sort of check seems needed here - for example, that transformers can only transform classes for their own web application where permission has been explicitly granted (perhaps based on ProtectionDomain). I'd think the ProtectionDomain should also be passed to any ClassFileTransformer called.

How does this interact with annotation scanning and ServletContainerInitializers?

For example, a transformer could add or remove annotations or class dependencies (e.g. adding new interfaces) that related to component scans or types handled by SCIs. Tomcat reads the bytecode directly for this, which would be different to bytecode returned from a transformed class and could result in ambiguous or inconsistent application behaviour.

Updated summary to indicate this is more about application driven bytecode weaving rather than Instrumentation (in the way the JRE defines instrumentation).

To address Jeremy's concerns/questions:

1) I don't see a security issue. JBoss, GlassFish, WebLogic, and WebSphere, for example, all "automatically enable this for all applications." You say, "Currently, that has to be specifically enabled by adding an agent to the JVM or by adding special classloader to the container installation and enabling it in a web application's Context," and I say that's a bad thing. It makes weaving difficult to configure, and sometimes impossible in hosted environments where the consumer doesn't have the ability to jump through these hoops.

2) I took some time to peruse the code on GlassFish, particularly  org.glassfish.web.loader.WebappClassLoader. It performs no special security checks. If someone calls addTransformer, it just lets them add the transformer. That's all. It doesn't use a ProtectionDomain, and it passes null to the transformer's transform method. JBoss also performs no security checks in its code, though it does pass the existing ProtectionDomain to the transformer. It's up to the transformer to use or not use the ProtectionDomain, but apparently it's allowed to be null.

3) If a user enables cross-context dispatch (off by default), they don't do so accidentally (they may do so ignorantly, but that's not our concern). Enabling cross-context dispatch already has its own whole set of security concerns, and I consider the possibilities with this minor compared to those other security concerns.

4) I don't have any particular objection to some sort of Tomcat setting (maybe in catalina.properties?) that disables/disallows transformation, though I strongly believe it should be enabled by default.

5) "How does this interact with annotation scanning and ServletContainerInitializers?" All of this scanning happens /before/ any application code is allowed to execute. As such, it happens before any code is capable of adding transformers. Thus, this won't affect such scanning at all. Transformers are only going to want to instrument classes that they use--that won't be SCIs. As far as adding/removing annotations, the creator of a transformer to be used in a web application knows better than to attempt to add or remove annotations that might be scanned for by the container. None of the other containers do anything to anticipate/detect/prevent this bad programming practice, which wouldn't work but also wouldn't hurt anything.

Now, to address what Rainer was saying about getThrowawayClassLoader(). After reading some GlassFish and JBoss source code, I've figured out what it's for. Code that adds transformers to a ClassLoader may need to initially load a class to determine whether it requires transformation (for example, Spring looks for classes annotated @Configurable and only adds a transformer for them if it finds a class annotated @Configurable). GlassFish implements this with a copy() method that simply returns a new UrlClassLoader with the same URL and parent class loader as the WebappClassLoader.

I've received some guidance from the folks over at SpringSource (https://jira.springsource.org/browse/SPR-10788) and I'd like to get started on a patch for this now. However, I've yet to hear from Mark whether he has a preference for adding a single addTransformer() method as I had originally suggested or adding registerTransformer() and unregisterTranformer() methods as Rainer suggested (or, a third alternative, addTransformer() and removeTransformer()).

Mark, what do you think? If you don't have a preference, I'll probably go Rainer's route but with addTransformer() and removeTransformer() as it's more flexible but also uses a convention (addTransformer()) that other containers use.

Thoughts?

(In reply to Nick Williams from comment #9)

> Mark, what do you think?

My primary concern has already been articulated in comment #1.

> If you don't have a preference, I'll probably go
> Rainer's route but with addTransformer() and removeTransformer() as it's
> more flexible but also uses a convention (addTransformer()) that other
> containers use.

I have a very small preference for addTransformer() and removeTransformer() but I'm happy to defer to the preferences of the Spring folks.

Mark

Okeydokey. addTransformer() and removeTransformer() is my preference and also what's easiest on the Spring folks (it will require no code change on their part to work), so that's the direction we'll go.

I noticed WebappClassLoader's toString() method includes a lot of information about the ClassLoader, such as the parent ClassLoader, context name, delegation setting, and repository. Should I also change toString() to append information about any attached transformers?

Created attachment 30678 [details]
Proposal for transformer registration API

(In reply to Nick Williams from comment #11)
> Okeydokey. addTransformer() and removeTransformer() is my preference and
> also what's easiest on the Spring folks (it will require no code change on
> their part to work), so that's the direction we'll go.

I'd suggest adding these in a separate interface for just this functionality so that applications don't need to depend on the actual implementation. The Tomcat API package (org.apache.tomcat) might be a good location.

Created attachment 30748 [details]
Proposed implementation of this feature

I have attached a patch with the implementation complete. In addition to 10 thorough unit tests, my functional testing with Spring Framework indicates that it works great.

The only remaining question I have, which may require a slight tweak, is regarding memory leaks. I can't figure out whether transformers added to the WebappClassLoader (which will come from application classes) constitute a potential memory leak. It seems to me that the WebappClassLoader instance is cleaned up when the application is shut down, so it shouldn't be a problem. But I wanted to get someone's opinion on whether the WebappClassLoader should clear out the contents of the "transformers" LinkedList when it shuts down. I'm not convinced it's necessary, so I left it out for now.

Created attachment 30749 [details]
Proposed implementation of this feature

There was an error in my previous patch. Please disregard and accept this one instead.

(In reply to Nick Williams from comment #15)
> Created attachment 30749 [details]
> Proposed implementation of this feature

1. Why loop over list rather than using contains() in addTransformer() ?

2. Should addTransformer() be looking for multiple instances of the same Transformer or multiple instances of the same class of Transformer?

3. Why not use List.remove(Object) in removeTransformer() ?

4. I'm concerned that synchronizing on the list of transformers while classes are transformed will become a bottleneck when lots of classes are being loaded and the transformer is relatively slow. A separate ReadWriteLock for the transformer list is probably the way to go but really some testing is required to determine if there is an issue here or not.

5. I'm not a fan of the org.apache.tomcat.unittest package unless the classes concerned are going to be used by multiple tests across multiple packages.

Created attachment 30825 [details]
Proposed implementation of this feature

(In reply to Mark Thomas from comment #16)
> 1. Why loop over list rather than using contains() in addTransformer() ?
>  
> 3. Why not use List.remove(Object) in removeTransformer() ?

That was my own mistake. I didn't read the Javadoc properly. I have corrected this in the attached revised patch.

> 4. I'm concerned that synchronizing on the list of transformers while classes are transformed will become a bottleneck when lots of classes are being loaded and the transformer is relatively slow. A separate ReadWriteLock for the transformer list is probably the way to go but really some testing is required to determine if there is an issue here or not.

Another mistake of mine. This could DEFINITELY be a problem if multiple threads are loading classes at the same time. A ReadWriteLock is definitely preferable over synchronization here. I have corrected this in the attached revised patch.

> 5. I'm not a fan of the org.apache.tomcat.unittest package unless the classes concerned are going to be used by multiple tests across multiple packages.

Understood. I have relocated/renamed these two classes in accordance with the discussion on the mailing list. The changes are in the attached revised patch.

> 2. Should addTransformer() be looking for multiple instances of the same Transformer or multiple instances of the same class of Transformer?

No, this was correct. It could be valid to have multiple instances of the same transformer class, but not multiple copies of the same instance.

An example use case is an application using JPA with Spring Framework. JPA abstracts away from the java.lang.instrument.ClassFileTransformer by specifying a javax.persistence.spi.ClassTransformer. JPA providers add ClassTransformers to the persistence unit instead of ClassFileTransformers. Applying this directly would require the ClassLoader to support javax.persistence.spi.ClassTransformer, which won't work in many cases (such as Tomcat). To get around this, Spring Framework uses a org.springframework.orm.jpa.persistenceunit.ClassFileTransformerAdapter to wrap a ClassTransformer with a ClassFileTransformer. If a provider adds multiple ClassTransformer implementations to the persistence unit, Spring will in turn add multiple ClassFileTransformerAdapter instances to the ClassLoader. All of these instances will do something different, but they will be of the same class as far as WebappClassLoader can tell.

1) CopyOnWriteArrayList would simplify managing added transformers and is intended for this use-case.

2) I would not try to de-dupe the list at all. There's no contract around equals() for transformers and this will be different from how JRE's Instrumentation manages them.

3) You're applying the transformers in findResourceInternal() which, I think, means getResource() will return transformed data. The transform should be moved into findClass() so only defined classes are transformed.

4) In the messages, we typically wrap [] around the substituted values i.e. [{0}]

(In reply to Jeremy Boynes from comment #18)
> 1) CopyOnWriteArrayList would simplify managing added transformers and is
> intended for this use-case.

How have I never heard of this class before? Certainly, mutation operations will definitely vastly outnumber traversal operations. Perhaps CopyOnWriteArrayList is the way to go here, instead of any locking. Mark? Thoughts?
 
> 2) I would not try to de-dupe the list at all. There's no contract around
> equals() for transformers and this will be different from how JRE's
> Instrumentation manages them.

You're correct, there's no contract for equals in ClassFileTransformer. By default a "duplicate" will be if the transformers are the exact same instance (==). Only if the person who implemented the transformer overrides Object#equals will a different behavior occur. Arguably, if the person overrides Object#equals, they likely did so to specify a different definition of "duplicate." I disagree that we shouldn't try to keep duplicates out of the list. I /would/ be open to saying that we should specifically look for duplicate instances (==) and not rely on Object#equals, in which case we need to go back to iterating over the list instead of using List#contains.

> 3) You're applying the transformers in findResourceInternal() which, I
> think, means getResource() will return transformed data. The transform
> should be moved into findClass() so only defined classes are transformed.

#findResourceInternal definitely feels like the right place for this. Re: "so only defined classes are transformed," how I've written it means only classes are transformed because the transformers are only applied if isClassResource is true. I don't see a problem here, but maybe I don't understand what you're saying.

> 4) In the messages, we typically wrap [] around the substituted values i.e.
> [{0}]

I would challenge "typically". Looks like about 50% of the time to me. If I'm being told that only [{0}] is right, I'll change it. But there is /plenty/ of code that isn't "correct" if this is the case. How are contributors to know which to use? They can't.

(In reply to Nick Williams from comment #19)
> (In reply to Jeremy Boynes from comment #18)
> > 1) CopyOnWriteArrayList would simplify managing added transformers and is
> > intended for this use-case.
> 
> How have I never heard of this class before? Certainly, mutation operations
> will definitely vastly outnumber traversal operations. Perhaps
> CopyOnWriteArrayList is the way to go here, instead of any locking. Mark?
> Thoughts?

As long as the solution is reasonable and meets the requirements there won't be any objection from me. I can't speak for any other committer.

> > 4) In the messages, we typically wrap [] around the substituted values i.e.
> > [{0}]
> 
> I would challenge "typically". Looks like about 50% of the time to me. If
> I'm being told that only [{0}] is right, I'll change it. But there is
> /plenty/ of code that isn't "correct" if this is the case. How are
> contributors to know which to use? They can't.

All new code uses [{0}] for clarity on what exactly the value is (makes it easier to spot empty values and unexpected white space). We are fixing older values slowly over time.

Mark

Created attachment 30838 [details]
Proposed implementation of this feature

Revised patch again. Switched to using CopyOnWriteArrayList, which should be much faster than synchronization or a ReadWriteLock to protect from concurrency issues. Wrapped [] around substitution values. Fixed mergability issues due to changes to WebappClassLoader in last couple of days.

I think this is very close. Juergen has been at SpringOne all week, but he should be home in the next day or two. Hopefully he can look at this soon now that things are less stressful over there. I'd like to get all the issues settled on the Tomcat side so that, once Juergen gives it the thumbs up, it can be committed ASAP. I'd really love to get this in 8.0.RC2 and 7.0.44, if at all possible.

The patch looks fine to me. Would be great to merge it into Tomcat proper as soon as possible.

Fortunately, since we're reflectively discovering those methods, we don't need to wait for Tomcat binaries to get Spring's part done: Based on the conventions in the patch, we'll adapt our discovery mechanism for Spring Framework 4.0 RC1, due on Oct 15th. Of course, it would be great if a corresponding Tomcat 8 release wasn't far beyond that...

Juergen

The patch has been applied with a few minor tweaks to fix the various warnings Eclipse threw up. It will be included in 8.0.0-RC4 onwards. I anticipate the next 8.0.0-RCx release around (maybe even before) the Spring 4 RC1 release.

Excellent. Thanks! Two things:

1) InstrumentableClassLoader, as committed, says "@since 8.0, 7.0.44." At the time I submitted my most recent patch, 7.0.44 was "next." As you know, due to several recent vote cancellations, 7.0.44 was actually never released. At the very least, it should say "@since 8.0, 7.0.44." Which brings me to point 2...

2) CAN this be included in 7.0.46? I'd like it to be. It will be some time before 8.0 obtains widespread adoption, and I don't see this as a breaking change, so I believe it would be beneficial to include it in 7.0.46. If it can't be in 7.0, then InstrumentableClassLoader just needs to be changed to say "@since 8.0."

*** At the very least, it should say "@since 8.0, 7.0.46."

The feature is backported to 7.0.x for 7.0.64 onwards.