Community
Participate
Working Groups
If I weave a class with Aspect1 using -Xreweavable then load-time weave it with Aspect2 (using org.aspectj.weaver.loadtime.Aj) then Aspect1 is not rewoven. This is confirmed by the lack of weaveinfo messages for Aspect1. This works for post-compile weaving although there are no explicit tests. Attached is a reweavable test for binary weaving. It needs to be adapted to run under org.aspectj.systemtest.ajc150.ataspectj.
Created attachment 24905 [details] Binary weaving -xreweavable test
no way to reproduce may be the aop.xml did not containted the Aspect1 ? (not included in test case) see test in AtAjLTWTests.testAjcAspect1LTWAspect2_Xreweavable based on your submission
The aop.xml files does not contain a declaration of the previously woven aspect and neither should it: the provider of an aspect library cannot possibly include the names of aspects he does not know about. The only reason we use -Xreweavable is because we cannot currently "overweave" an aspect. This issue should be transparent to the user (that's why the option will be the default). The behaviour of LTW should be exactly the same as binary weaving: if a class has already be woven we get the original bytecode, the list of previously woven aspects, the list of new aspsects then weave them all together. I have taken a look the the testcase you mention and do no believe it covers the case I am describing. There need to be 2 separate compile steps with different aspects. I will convert the binary weaving testcase to the LTW framework.
the test case is fine. It does compile with ajc -Xreweavable Main and Aspect1, build the outjar main1.jar, then javac on Aspect2 and then LTW based on Aspect2.class and main1.jar (you might not be used to my Ant driven LTW framework but have a look at the sandbox if you like). I had already some other use cases for that anyway The feature you describe is appealing - I have been thru during the implementation. One issue that arise and that you seem to forget : at any point of time in what you suggest the set of aspect for a given classloader may change, since we will discover some aspects as we load reweavable classes. This means that for such a discovered aspect, it won't be applied to the set of classes exposed to the LTW infra if -Xreweavable was not there ie that the weaving is NOT idempotent (since some classes may have already been loaded when we discover the aspect and we cannot change them anymore) How would you suggest to implement that ? I did try and its not possible in any way - aside making the beast more complex by triggering new system update as per aspects are discovered (new precedence, supposely new cflow counters etc etc) I think a best option hanging around is to generate the aop.xml as part of outjar / ajc (see open issue on that).
just throwing in a thought here....the problem seems to be that at any time we may 'come across' a class that was built reweavable - that can cause a new aspect to be brought into the world which must be included in the weaving step for this class. That aspect then hangs around and potentially also affects classes woven later that weren't originally intended to be targeted by it. We could, on bringing in an aspect because of encountering a reweavable class, tag it as such. Then, matching could have a slight change to say: boolean matched = false; if (typemunger/shadowmunger matches joinpoint) { matched = true; if (munger.declaringType() is tagged as being here because of being bought in due to encountering a reweavable class) { if (!(joinpoint.containingclass.reweavableAspectList contains munger.getdeclaringType()) { matched=false; } } } effectively matching is changed slightly to include a check that if the munger came from an aspect brought in and tagged as 'existing due to reweavable behavior' then we only allow the match if the aspect is listed in the reweavable set that was recorded in the matched class when it was originally woven... i haven't put hours and hours of thought into this ...
That 's a solution but the main issue with that is the idempotent property. All that are hacks around reweavable. I claim that an aspect that says "match all calls to *.foo()" must do so no matter how it is weaved in, and if it happens that the program was compiled and weaved with ajc while ommitting some other modules / dependancies that I will finally use in my (LTW enabled or not) runtime, then the weaving script / compilation process is broken or my pointcut is badly written and unsafe. We could as you suggest add an implicit filter to narrow down the scope of the weaved in extracted aspect to the class from which we have extracted it thanks to reweavable, but that would mean that a "match all calls to *.foo()" would depend on the ajc input when reweavable is used. Furthermore, if we do so and ones do want to "match all calls to *.foo()" in the LTW, he would have to add another new aspect but making sure that he handles to write a pointcut that does not intersect with the previously set of (ajc) compiled classes - which is impossible. What we can do is to make sure proper message gets issued from LTW when an unregistered aspect (not in aop.xml) is found in some reweavable bytecode. Note that any user can then add its aop.xml if this one was missing (it just need to be in the same classloader level).
Interesting discussion around this today. I just want to capture some of the issues here. Firstly, I think the semantics we always want to aim for are those we would have with a full AOVM (eg. if i load an aspect that affects *everything* in such a world, then it should affect everything). All the weaving etc. we do ahead of loading by the VM is simply an approximation to the AOVM semantics in the case where we just have a dumb-old VM. That said, imagine I put a third-party jar on my classpath which contains an aspect saying "weave everything". At the time that jar was built, "everything" was the content of the third-party library, but now, "everything" is much more than that. We said that the semantics must be that "everything" means *everything*. But we sure would like a way to control such aspects (otherwise I can write a trojan aspect in my utility jar that say uses around advice to bypass security calls, and weave just some little class in my own jar with it (lets call that the trigger class), in reweavable mode - now when we want to reweave the trigger class for any reason, the trojan aspect escapes and affects the whole system without us knowing. This would clearly be bad. So if we stick to the AOVM model as defining the ideal semantics, then what seems to be missing to me is a new security manager privilege or set of privileges that determine whether or not an aspect can affect some type. These privileges would logically be granted based on domains (eg. can only weave org.xyz..*). In the absence of the AOVM we need somehow to emulate such a security manager and the associated privileges in our aop.xml and LTW implementation. In a nutshell, an aspect that wants to advise everything does advise *everything*, but a security manager may prevent it from doing so when in tries.
for such a trojan aspect: 1/ classloader rules of LTW gives a 1st security level. If ones put a trojan aspect, it means he has control on your classpath (web app classpath eventually ie classpath from within a classloader or deployment unit - or whole app server system classpath - no matter the use case is the same). So the breach is elsewhere (it is in your config / deployer roles) since one could certainly remove some classes, change manually the bytecode etc etc. Nothing to do with aspects. 2/ if this is a concern, drop an aop.xml in the deployed unit and say "<exclude within="...."> and we will respect that - no matter the trojan aspect says it weaves everything 3/ yes, ones could come with a more advanced schemes, where there is a much finer control on what is deployed and where stuff is weaved and why. We talked about that when we presented LTW at AOSD 04 (we called that AOP container). Unfortunately the current level of technology and adoption does not lurk for that yet.
As it stands today the system is broken. Two people, separated in either time or space, can write an AO application and an LTW-enable aspect library respectively that work fine in isolation but fail silently when brought together. The LTW framework will simply unweave the existing aspect from the application and it will then behave incorrectly. There seem to be 3 solutions on the table: 1. Add the reweavable aspect to the aop.xml file for the application JAR/bundle. This is no good for 3 reasons: a. The application programmer shouldn’t have to know how his application is to be used and even if “-outjar” builds an aop.xml automatically he might not use that option. b. The final deployer of the application, who may not be the same person that wrote the aspect library, will not understand the failure so therefore will not know to update or create an aop.xml. c. Declaring the aspect in aop.xml will expose it beyond its defining class loader making what was a private aspect very public. This may need additional configuration in an OSGi environment even to get the system to work. This raises the subject of public/private aspects in a componentized world which may need further consideration. 2. As Andy suggests limit the scope of a reweavable aspect. However this will require some work and as Alex points out the affect of an aspect shouldn’t depend on the way you built your application. 3. As Adrian suggests think in terms of an AOVM. If a pointcut is written to match everything then it should. However while we exist in a compile- time/load-time duality we need to account for the intent of the aspect writer. My suggested solution is as follows. When a reweavable aspect is found it should be added to the set for the classloader. This will ensure the application behaves correctly. However this is not idempotent so we must issue a warning which given the right configuration can allow the system to fail. I don’t believe this is a serious problem as in most cases all of the classes in a JAR/bundle will have been exposed to an aspect even if they were not woven with it. The important thing is we limit the scope of any additional weaving.
Created attachment 25037 [details] Simple binary-weaving testcase There are 3 separate tests. If the 1st reweavable aspect is either packaged with the affected classes or supplied on the aspectpath when the 2nd aspect is woven then the test passes. If the 1st aspect is merely on the classpath, as it would be in an LTW scenario, the test fails (no weaveinfo messages).
Matthew could you sum up here your suggested solution ? As well as proposing some bits for the implementation ? If I have to choose one except the "aop.xml is mandatory" as we did defined and practiced since 2 years in both AW and JBoss (and is it will probably be if an AOVM pops up), I would choose Andy proposal (ie narrow the scope of the aspect to match only the class in which we find it in the reweable info) - the one you noted "2" in your previous comment. I would delay that one to M4 - since it has serious implication on the architecture : - weave based on a regular world and an "extra-world" that would contain the narrowed aspect with reorganized precedence etc - perThis/perTarget and ajc$MightHave aspect to be thought some - what if the added aspect is already in the aop.xml - what if a different version is found [ie someone has upgraded the aspect that we find in reweavable (and what is an incompatible version vs just a different one we accept - only aspect class name ?] If I have to seriously veto one, I would veto the one that consist in "just registering the aspect as it is and for all subsequent class loads" when we find it in some reweavable bits, since it precisely means that the weaver behavior will depend on the application flow ie is not predictable at all. (retarget to M4 anyway)
There is only a problem with option (3) _if_ the pointcut is badly written and even then it will only affect classes in the same namespace. If we do nothing to solve this problem in M3 we must at least issue a warning as currently applications will fail silently when aspects they were originally woven with are thrown away because they were not named in aop.xml. I will work on a patch.
Created attachment 26902 [details] Patch and testcase
I have created a patch to issue an error if a reweavable aspect is not declared in an aop.xml file. This must be an error not a warning, because woven applications will not behave properly if they are unwoven at loadtime. The error is issued by the BcelWeaver, in the processReweavableStateIfPresent method, if it discovers an aspect which was previously woven into a class, but is not declared in some aop.xml file. This ensures that the user is told that their application may behave incorrectly. I have uploaded a zip containing a patch to the weaver project, and a patch to the tests project containing a new loadtime weaving test for this error message.
Just a quick comment about this: I am worried about any solution that won't let you compose different modes of using AspectJ. In particular, if someone wants to use build-time aspects on an application, and then apply load-time weaving to add additional aspects, it's critical that these things work together. I think this is what Matthew was saying earlier. Generating an aop.xml file when building is helpful, but I don't think there's any reason to expect that people deploying build-time woven applications will in fact include such files. This is another example of the antipattern of not building in composability (just like defaulting to generating not reweavable aspects). It's going to cause major problems when more than one component uses aspects and they don't interoperate. I would further argue that discovering already woven reweavable aspects that aren't exposed in an aop.xml should NOT behave like inpath entries: they do NOT need to affect anything else at runtime and in fact should not. I'd like to see the load-time weaving defined aspects be the only ones that have an effect. I appreciate the complexity of making a design that has some of the basic constraints desired but I strongly support Matthew's view of them. There are many scenarios where people would like to add an aspect to a system through LTW and can't reasonably expect to know what other aspects are deployed. These include monitoring systems (like Glassbox) and enhanced testing frameworks. Given the choice between perfect semantics and reasonable componentization, I strongly suppott the latter.
I think we are faced with the issue that "over weaving" i.e. weaving woven code is just not reliable. However we have not lost sight of the need to more transparently reweave aspects when load-time weaving new ones: it's just unlikely to make AspctJ 5 final. The most recent patch is to prevent woven applications from failing silently. I think wnat we are ultimatley aiming for is to always weave at load-time (just like late binding for classes). The first step is better support for reweaving without the need for aop.xml entries and the resulting scope creep. For the moment we need something that works and information when it doesn't.
I appreciate the complexity and agree that it's not worth delaying the 1.5.0 release to create the ideal solution. So my naive idea is to just reweave build-time aspects into the types where they originally applied at the same time you weave load-time aspects, i.e., never apply build-time aspects to a type unless they were already applied previously. The only scenario where I could imagine possibly surprising behavior would be if a load-time aspect interacts with a build-time aspect (e.g., requiring advice precedence, generating a conflicting ITD, etc.). In those cases, just reweaving with normal rules seems reasonable. What's wrong with that approach?
There is nothing wrong with the approach. However each type would essentially need its own mini-world, with its own set of aspects. See comment #5.
raising to P2
commited David Knibb patch for error message refactoring for a world per weaved class to be decided (post 1.5? performance impact? etc)
*** Bug 108888 has been marked as a duplicate of this bug. ***
Our (Andy & I) understanding of this bug is that all the work planned to be done for 1.5.0 is now complete, but there remain some important improvements that would be desirable post 1.5.0. I'm moving the enhancement to 1.5.1 to capture those. Matthew/Alex if you believe there *is* something further we have to do before 1.5.0 RC1 please say. Otherwise, it would be nice to continue the discussion in this bug to get a clear description of what we need to do in this area in 1.5.1. Again Matthew/Alex please could you drive that. THanks, Adrian.
+1 post 1.5.0 as current behavior can be upgraded without pain for users
+1 We have a good workaround and an error to ensure users know what to do.
is there something to document here? or have we now done enough? review for 1.6.0
