according to https://rules.sonarsource.com/java/tag/leak/RSPEC-5164/. Now, there no longer is a thread-local stack of AroundClosure instances, but rather a list of them, which can only grow but never shrink. Instead, we now have a thread-local (integer) list index, for every thread being initialised with pointing to the last element. I.e., every thread can unwind by decrementing the index while proceeding, independently of other threads. A positive side effect is that this approach also works for long-lived threads from thread pools, used by executor services. Hence, test Bugs199Tests.testAsyncProceedNestedAroundAdviceThreadPool_gh128, which was previously commented out, has been activated and passes, see #141. I am not sure if this brings @AspectJ style, non-inlined, nested around advice execution functionally on par with native ones, but at least for current scenarios it seems to work. Fixes #288, #141. Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>tags/V1_9_21_2
package org.aspectj.runtime.reflect; | package org.aspectj.runtime.reflect; | ||||
import java.util.Stack; | |||||
import org.aspectj.lang.JoinPoint; | import org.aspectj.lang.JoinPoint; | ||||
import org.aspectj.lang.ProceedingJoinPoint; | import org.aspectj.lang.ProceedingJoinPoint; | ||||
import org.aspectj.lang.Signature; | import org.aspectj.lang.Signature; | ||||
import org.aspectj.lang.reflect.SourceLocation; | import org.aspectj.lang.reflect.SourceLocation; | ||||
import org.aspectj.runtime.internal.AroundClosure; | import org.aspectj.runtime.internal.AroundClosure; | ||||
import java.util.ArrayList; | |||||
import java.util.List; | |||||
class JoinPointImpl implements ProceedingJoinPoint { | class JoinPointImpl implements ProceedingJoinPoint { | ||||
static class StaticPartImpl implements JoinPoint.StaticPart { | static class StaticPartImpl implements JoinPoint.StaticPart { | ||||
String kind; | String kind; | ||||
} | } | ||||
} | } | ||||
static class InheritableThreadLocalAroundClosureStack extends InheritableThreadLocal<Stack<AroundClosure>> { | |||||
@Override | |||||
protected Stack<AroundClosure> initialValue() { | |||||
return new Stack<>(); | |||||
} | |||||
@Override | |||||
protected Stack<AroundClosure> childValue(Stack<AroundClosure> parentValue) { | |||||
return (Stack<AroundClosure>) parentValue.clone(); | |||||
} | |||||
} | |||||
Object _this; | Object _this; | ||||
Object target; | Object target; | ||||
Object[] args; | Object[] args; | ||||
// will either be using arc or arcs but not both. arcs being non-null | // will either be using arc or arcs but not both. arcs being non-null | ||||
// indicates it is in use (even if an empty stack) | // indicates it is in use (even if an empty stack) | ||||
private AroundClosure arc = null; | private AroundClosure arc = null; | ||||
private InheritableThreadLocalAroundClosureStack arcs = null; | |||||
private List<AroundClosure> arcs = null; | |||||
private final ThreadLocal<Integer> arcIndex = ThreadLocal.withInitial(() -> arcs == null ? -1 : arcs.size() - 1); | |||||
public void set$AroundClosure(AroundClosure arc) { | public void set$AroundClosure(AroundClosure arc) { | ||||
this.arc = arc; | this.arc = arc; | ||||
} | } | ||||
public void stack$AroundClosure(AroundClosure arc) { | |||||
public void stack$AroundClosure(AroundClosure arc) { | |||||
// If input parameter arc is null this is the 'unlink' call from AroundClosure | // If input parameter arc is null this is the 'unlink' call from AroundClosure | ||||
if (arcs == null) { | if (arcs == null) { | ||||
arcs = new InheritableThreadLocalAroundClosureStack(); | |||||
arcs = new ArrayList<>(); | |||||
} | } | ||||
if (arc==null) { | |||||
this.arcs.get().pop(); | |||||
} else { | |||||
this.arcs.get().push(arc); | |||||
if (arc == null) { | |||||
arcIndex.set(arcIndex.get() - 1); | |||||
} | |||||
else { | |||||
this.arcs.add(arc); | |||||
arcIndex.set(arcs.size() - 1); | |||||
} | } | ||||
} | |||||
} | |||||
public Object proceed() throws Throwable { | public Object proceed() throws Throwable { | ||||
// when called from a before advice, but be a no-op | // when called from a before advice, but be a no-op | ||||
return arc.run(arc.getState()); | return arc.run(arc.getState()); | ||||
} | } | ||||
} else { | } else { | ||||
final AroundClosure ac = arcs.get().peek(); | |||||
final AroundClosure ac = arcs.get(arcIndex.get()); | |||||
return ac.run(ac.getState()); | return ac.run(ac.getState()); | ||||
} | } | ||||
} | } | ||||
public Object proceed(Object[] adviceBindings) throws Throwable { | public Object proceed(Object[] adviceBindings) throws Throwable { | ||||
// when called from a before advice, but be a no-op | // when called from a before advice, but be a no-op | ||||
AroundClosure ac = null; | |||||
if (arcs == null) { | |||||
ac = arc; | |||||
} else { | |||||
ac = arcs.get().peek(); | |||||
} | |||||
AroundClosure ac = arcs == null ? arc : arcs.get(arcIndex.get()); | |||||
if (ac == null) { | if (ac == null) { | ||||
return null; | return null; |
} | } | ||||
public void testAsyncProceedNestedAroundAdviceThreadPool_gh128() { | public void testAsyncProceedNestedAroundAdviceThreadPool_gh128() { | ||||
// TODO: future improvement, see https://github.com/eclipse-aspectj/aspectj/issues/141 | |||||
// runTest("asynchronous proceed for nested around-advice (@AspectJ, thread pool)"); | |||||
// Test created for #128, but initially commented out and remaining work recorded in #141. | |||||
// Now, test is expected to pass. See https://github.com/eclipse-aspectj/aspectj/issues/141. | |||||
runTest("asynchronous proceed for nested around-advice (@AspectJ, thread pool)"); | |||||
} | } | ||||
public void testAsyncProceedNestedAroundAdviceNative_gh128() { | public void testAsyncProceedNestedAroundAdviceNative_gh128() { |
</ajc-test> | </ajc-test> | ||||
<ajc-test dir="bugs199/github_128" title="asynchronous proceed for nested around-advice (@AspectJ)"> | <ajc-test dir="bugs199/github_128" title="asynchronous proceed for nested around-advice (@AspectJ)"> | ||||
<compile files="Application.java MarkerA.java MarkerB.java annotation_syntax/MarkerAAspect.aj annotation_syntax/MarkerBAspect.aj" options="-1.8" /> | |||||
<compile files="Application.java MarkerA.java MarkerB.java annotation_syntax/MarkerAAspect.aj annotation_syntax/MarkerBAspect.aj" options="-1.8 -XnoInline" /> | |||||
<run class="Application" options="1,1"> | <run class="Application" options="1,1"> | ||||
<stdout ordered="no"> | <stdout ordered="no"> | ||||
<line text=">> Outer intercept"/> | <line text=">> Outer intercept"/> | ||||
</ajc-test> | </ajc-test> | ||||
<ajc-test dir="bugs199/github_128" title="asynchronous proceed for nested around-advice (@AspectJ, thread pool)"> | <ajc-test dir="bugs199/github_128" title="asynchronous proceed for nested around-advice (@AspectJ, thread pool)"> | ||||
<compile files="Application.java MarkerA.java MarkerB.java annotation_syntax/MarkerAAspect.aj annotation_syntax/MarkerBAspect.aj" options="-1.8" /> | |||||
<compile files="Application.java MarkerA.java MarkerB.java annotation_syntax/MarkerAAspect.aj annotation_syntax/MarkerBAspect.aj" options="-1.8 -XnoInline" /> | |||||
<run class="Application" options="1,1,true"> | <run class="Application" options="1,1,true"> | ||||
<stdout ordered="no"> | <stdout ordered="no"> | ||||
<line text=">> Outer intercept"/> | <line text=">> Outer intercept"/> | ||||
</ajc-test> | </ajc-test> | ||||
<ajc-test dir="bugs199/github_128" title="asynchronous proceed for nested around-advice (native)"> | <ajc-test dir="bugs199/github_128" title="asynchronous proceed for nested around-advice (native)"> | ||||
<compile files="Application.java MarkerA.java MarkerB.java native_syntax/MarkerAAspect.aj native_syntax/MarkerBAspect.aj" options="-1.8" /> | |||||
<compile files="Application.java MarkerA.java MarkerB.java native_syntax/MarkerAAspect.aj native_syntax/MarkerBAspect.aj" options="-1.8 -XnoInline" /> | |||||
<run class="Application" options="1,1"> | <run class="Application" options="1,1"> | ||||
<stdout ordered="no"> | <stdout ordered="no"> | ||||
<line text=">> Outer intercept"/> | <line text=">> Outer intercept"/> | ||||
</ajc-test> | </ajc-test> | ||||
<ajc-test dir="bugs199/github_128" title="asynchronous proceed for nested around-advice (native, thread pool)"> | <ajc-test dir="bugs199/github_128" title="asynchronous proceed for nested around-advice (native, thread pool)"> | ||||
<compile files="Application.java MarkerA.java MarkerB.java native_syntax/MarkerAAspect.aj native_syntax/MarkerBAspect.aj" options="-1.8" /> | |||||
<compile files="Application.java MarkerA.java MarkerB.java native_syntax/MarkerAAspect.aj native_syntax/MarkerBAspect.aj" options="-1.8 -XnoInline" /> | |||||
<run class="Application" options="1,1,true"> | <run class="Application" options="1,1,true"> | ||||
<stdout ordered="no"> | <stdout ordered="no"> | ||||
<line text=">> Outer intercept"/> | <line text=">> Outer intercept"/> |