Community
Participate
Working Groups
I have reproduced a simple test case where LTWeaving is failing to inline around advice that build time weaving inlines. A patch with a test case will follow. This is a significant problem because of issues in defining classes in load-time weaving where we ran into this in applying a caching aspect. Notably, I have encountered a bug in WebSphere 6 whereby the context ClassLoader is set to the bootstrap ClassLoader when loading application classes, which breaks any weaving plugin (like my contribution) that tries to use the context ClassLoader for weaving (e.g., to define classes). Of course, this bug can cause other issues too, but it definitely causes problems in around advice that isn't inlined (since the generated class is defined in the bootstrap loader, it can't see the application class loader type!)
Created attachment 45406 [details] Test that detects the presence of closures with a reflection test. I also verified the discrepency manually by using the <dump within="*"/> flag and running javap on the load-time woven class which doesn't inline advice that the build-time woven class does...
I stepped through this test case in a debugger and found that at the key point BcelAdvice.canInline returns false because BcelWorld.getBcelObjectType(concreteAspect).getLazyClassGen() generates a new unwoven lazy class gen object. I'm not sure how this ought to be fixed... it seems like the lazy class gen ought to be loaded from the bytecodes when the aspect is registered, since in general you can't rely on aspects being woven again in load-time weaving (and currently aspects aren't woven whether the aspect ought to have been woven when registering). This bug may be related to bug #133770, which is another case of load-time weaving not performing required work that is done by build-time weaving.
my initial reaction is that its a bit flaky to create a system that relies on a 'possible' compiler optimization in order for the code to work properly... but I will investigate it
I agree in theory, but the reality is that load-time weaving defines closures by using reflection's accessible object to call a protected ClassLoader method to define classes. I don't see any good alternative implementation for that. But there's no reason to expect that ClassLoaders should support this type of call to a non-public method and in practice Matthew has often counciled people to avoid writing around advice that can't be inlined. If we had good built-in support for AOP in the VM, this wouldn't be an issue, but we're stuck engineering the best support we can given the state of Java VM's. I do agree that the WebSphere bug makes it more challenging to use load-time weaving on WebSphere 6.0 and earlier, until you get to Java 5 with -javaagent support, but I believe this support is still quite useful to achieve...
Reading Bug 119657 might be instructive. We always endeavour to inline around advice unless -XnoInline is specified. However there are situations when inling is not possible. Certain types of advice where dispatch is modified for example. The other occurs with LTW. We cannot inline around advice until the aspect that defines it is itself woven. This doesn't happen until the aspect is defined which is typically _after_ the first class it weaves is defined. This may only apply to aspects that also weave themselves but I cannot be sure. We may need to expand the testcases I wrote to verify this. Does the aspect implicated in this bug weave itself?
re. the WebSphere bug, that sounds very nasty. Presumably that will also break things like Hibernate that use cglib...
In the test case, I verify that the same advice for the same aspect is inlined in build-time weaving and not in compile-time weaving. The inlining isn't happening because the aspect hasn't been woven to generate the lazyClassGen when advising the class. I think it's important to inline consistently from when the first class is loaded. Would it be a problem to reweave aspects eagerly right after calling prepare for weave? Having a limitation whereby non-rewoven aspects can't inline advice also interacts badly when you want to not reweave aspect libraries. I often don't rewave them for efficiency or to work around other limitations such as with concrete xml aspects (e.g., if they include internal aspects that don't need to be applied to non-library code).
Re: the WebSphere bug, indeed it is a nasty one. I added print statements to the ClassLoader plugin to show the context ClassLoader during the preDefineApplicationClass method and was indeed surprised to see that it was set to the bootstrap ext loader, not the application class loader. I found another bug report about this same problem only here it was observed during staticinitialization: http://mail-archives.apache.org/mod_mbox/portals-jetspeed-dev/200606.mbox/%3C4034668.1149777210719.JavaMail.jira@brutus%3E This bug also explains why people using the WasWeavingPlugin contribution had failures to resolve around closures in JasperLoaders: the Tomcat JasperLoader won't delegate to the context loader to resolve classes that are defined in the JSP package (as, of course, these closures are). So the class gets generated but can't be resolved...
There are 3 separate issues here which I will deal with separately: 1. We do not always inline around advice during LTW when we would have done for compile or post compile-time weaving 2. Defining closures during LTW is problematic because unless we own the class loader we need to invoke a protected method reflectively 3. There is a bug in the WAS 6.0 implementation of the semi-public WebSphere Classloader Plugin interface which requires you to use the Context Classloader (because the defining loader is not explicit) and this is not set correctly. Issues 2 & 3 are outside the scope of this bug and the discussion is just noise. Let's concentrate on 1. When we weave a class with around advice offline we _may_ generate a closure. We cannot use this implementation during LTW because we cannot guarantee that it will be there and we may end up with duplicate as in Bug 119657. We cannot inline around advice until we have woven the aspect that defines it because the aspect itself may be woven by another aspect. We cannot weave an aspect until it is loaded, typically in response to a class that it affects being defined. This means that for LTW, even if we could inline around advice, we will generate at least one closure. >This bug may be related to bug #133770, … It’s not. >Would it be a problem to reweave aspects eagerly right after calling prepare for weave? Maybe not, but I suspect it’s too late for 1.5.2. Andy?
Ron, >I'm not sure how this ought to be fixed... it seems like the lazy class gen ought to be >loaded from the bytecodes when the aspect is registered, since in general you can't rely >on aspects being woven again in load-time weaving (and currently aspects aren't woven >whether the aspect ought to have been woven when registering). >Having a limitation whereby non-rewoven aspects can't inline advice also >interacts badly when you want to not reweave aspect libraries. I often don't >rewave them for efficiency or to work around other limitations such as with >concrete xml aspects (e.g., if they include internal aspects that don't need to >be applied to non-library code). Both of these comments concern me. ALL types defined by a weaving class loader MUST be reweavable. I assume that your library aspects are defined using a non-weaving class loader. If you are achieving this with include/exclude in the aop.xml then you are taking a risk. There is however a 4th issue in here which is worth discussing. If we have an aspect that defines around advice that can be inlined and we can guarantee that the aspect will never be rewoven then we should be able to always inline the advice and not use the unwoven version of the aspect as we have had to do as a result of the fix to Bug 119657. The important word in the last sentence is “guarantee” because it is something that, from the point of view of the weaver at least, we cannot yet do.
I think this "bug" is an ehancement request (or possibly two requests to address issues #1 and #4) because the code is working as intended. As Andy points out inlining around advice is a compiler optimization that cannot be relied upon. I suggest we review the generation of closures during LTW in 1.5.3.
I'm attempting to use LTW at a client with Websphere 5.1 here. I use around advice and have seen the closure problem mentioned in this bug. I have been using a short term workaround of setting the WAR class loader policy in WAS 5.1 to 'application' instead of 'module'. Thus far, we have not found an acceptable long term work-around (other than moving up to Java 5 with -javaagent) -- which is not going to be on the table around here likely for another year (with a Websphere 6.1 upgrade). There are certainly other ways I can attack this problem such as hacking/replacing the VM's classloader, but if you could [at load-time] inline my around advice to begin with, it would be appreciated by many developers here.
So if I understand you correctly when WasWeavingPlugin.preDefineApplicationClass() is invoked by WAS 6.0 the context class loader in _not_ the application class loader but in fact the extensions class loader. If this is true then you have much bigger problems than trying to define generated closures. The rule for AspectJ LTW is that each class loader _must_ have its own weaver instance. This is because each class loader has its own namespace allowing multiple copies of Foo. The Aj interface achieves this by keeping a WeakHashMap ClassLoader->ClassLoaderWeavingAdaptor. With the WAS 6.0 bug you are only using one weaver and throwing all your types into it. Worse still you will only get one set of aspects. I am puzzled by this observation however because I can’t understand how the weaver would resolve any WAS-specific classes which if I remember correctly are loaded from CLASSPATH. Maybe your application doesn’t use any. Also where are your aspects defined? If they are with your application I don’t see how they would ever be loaded. I strongly suggest you double check by collecting verbose output from AspectJ. You should get a registration call for each application class loader along with the aspects defined. It might be that WAS only gives you the wrong class loader some of the time.
The verbose output showed multiple loaders being set, so the bug appears to be intermittent rather than consistent, i.e., the context class loader is often set correctly. However, in the cases we were testing, we were deploying aspects to the system classpath to be shared across multiple applications, which can avoid problems that the bug would cause in deploying application-specific aspects. Matthew Smith: at what level are you defining your aspects? Are they deployed to the app or to the system classpath?
Ron, While that helps to explain the situation it doesn't make it any better! If you define any application specific aspects then you may break. If you need any applicaton specific types during weaving then you may break. If you use Xlint to ignore "Can't find type" then ... Until the WAS 6.0 bug is fixed then from a reliability perspective the WasWeavingPlugin has a limited (but useful) capability. I _strongly_ suggest you document these limitations (or I will) and if possible enforce them. Otherwise I am likely to be very suspicious when I see LTW bugs raised against AspectJ when the WasWeavingPlugin is in play.
I agree that it's important to document the limitation due to this WebSphere bug. I'd like to better understand when it occurs and hopefully to raise an issue with IBM about it (at least an issue with respect to having the wrong ClassLoader at staticinitialization time - rather than trying to get support on the CL plugin API).
so ... I see lots of discussion, have we come to any conclusions? Regarding this comment: > >Would it be a problem to reweave aspects eagerly right after calling prepare for weave? > Maybe not, but I suspect it’s too late for 1.5.2. Andy? too late for 1.5.2, yes. Batch compilation always weaves aspects first. What you do for LTW, I dont know - didn't write that code ;) until the aspect is woven and so any joinpoints in the advice are affected then we can't extract that advice and 'stick it' somewhere else (ie. inline it). I can't think of a clean way to mark the advice in the aspect as 'wont be woven' - making it safe for inlining from the very beginning of the process. Anyone suggesting another 'user option' to configure this will be shot...
We should close this bug as WAD. However the issues discussed, especially WRT the WASWeavingPlugin, create a small use-case for one of 2 possible enhancements: 1. As Ron suggests could we “weave” an aspect before we actually load it, possibly during prepareForWeave(), so that we could inline any around advice. 2. As I suggest _if_ we can guarantee that an aspect will not we woven could we use the already woven byte-code and inline any around advice.
The elegant solution, which in this case is compensating for a WebSphere bug (although it also avoids calling ClassLoader.defineClass() which has Java 2 security implications), is to weave aspects up front so I have opened an enhancement: Bug 149802.
