Summary: | StackOverflowError on circular pointcut iff aspect parameterized | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | [Tools] AspectJ | Reporter: | Wes Isberg <wes> | ||||||
Component: | Compiler | Assignee: | Helen Beeken <hlhawkins> | ||||||
Status: | RESOLVED FIXED | QA Contact: | |||||||
Severity: | normal | ||||||||
Priority: | P2 | ||||||||
Version: | 1.5.0M4 | ||||||||
Target Milestone: | 1.5.0RC1 | ||||||||
Hardware: | PC | ||||||||
OS: | Windows XP | ||||||||
Whiteboard: | |||||||||
Attachments: |
|
Description
Wes Isberg
2005-11-06 07:26:57 EST
I've managed to recreate the problem with the following simplified testcase: abstract aspect ParameterizedAbstractAspect<T>{ abstract protected pointcut pc(); before() : pc() {} } aspect ParameterizedConcrete extends ParameterizedAbstractAspect<ParameterizedConcrete> { protected pointcut pc() : pc(); } The reason the StackOverflowError is occuring is in ReferencePointcut.concretize1(...). In this method, there is a flag called "concretizing" which is initially false but is set to true when concretizing is taking place. The method begins with a check on whether or not "concretizing" is true and if it is then the expected error message is produced - 'circular pointcut declaration involving: pc()' type message. If it's false, then we go ahead and try to concretize. The part of the method which is causing the problems is: Pointcut ret = pointcutDec.getPointcut(); if (typeVariableMap != null) ret = ret.parameterizeWith(typeVariableMap); return ret.concretize(searchStart, declaringType, newBindings); (note that the implementation of concretize calls the concretize1 method). If the aspect is not parameterized then typeVariableMap == null. However, if it is parameterized then typeVariableMap is non-null. In this case, ret becomes a new object (due to the implementation of ReferencePointcut.parameterizeWith(..)). So, for example, if we're looking at non-parameterized aspects, we get the following info at the trouble spot in concretize1: first pass in concretize1: this: id = 144 concretizing is set to true, ret : id = 176 concretizing is still false second pass in concretize1: this: id = 176 concretizing is set to true ret : id = 176 concretizing is still true third pass in concretize1: this: id = 176 concretizing is true so get compiler warning as expected However, if we're looking at parameterized aspects, we get the following: first pass in concretize1: this : id = 47 concretizing is set to true ret (before call to parameterizeWith(..) : id = 92 concretizing is false ret (after call to parameterizeWith(..) : id = 95 concretizing is false second pass in concretize1: this : id = 95 concretizing is set to true ret (before call to parameterizeWith(..) : id = 92 concretizing is false ret (after call to parameterizeWith(..) : id = 135 concretizing is false third pass in concretize1: this : id = 135 concretizing set to true ret (before call to parameterizeWith(..) : id = 92 concretizing is false ret (after call to parameterizeWith(..) : id = 147 concretizing is false this goes on and on..... and hence the StackOverflowError. The fix is to add a protected boolean field hasBeenParameterized to the abstract class Pointcut. Then add an extra check in the problem code mentioned in comment #1, changing it from if (typeVariableMap != null) ret = ret.parameterizeWith(typeVariableMap); to if (typeVariableMap != null && !hasBeenParameterized) { ret = ret.parameterizeWith(typeVariableMap); ret.hasBeenParameterized=true; } It is necessary to update the field here rather than in the ReferencePointcut.parameterizeWith(...) method, because parameterizeWith is called several times from different places whereas you only concretize once. Created attachment 29852 [details]
patch containing fix
Apply the patch to the weaver project.
The patch contains the fix described in the previous comment.
Created attachment 29853 [details]
patch containing testcases
Apply this patch to the tests project.
This patch contains the original failing testcase in file pr115325.aj. I've
also included another testcase in file pr115325b.aj which came about from me
investigating difference scenarios. I'm not sure whether it should be included
in the suite, however, I thought I would include it in the patch just in case
:-)
fix available. |