Avoid ThreadLocal memory leak in JoinPointImpl

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>

diff --git a/runtime/src/main/java/org/aspectj/runtime/reflect/JoinPointImpl.java b/runtime/src/main/java/org/aspectj/runtime/reflect/JoinPointImpl.java
index 851a8b3dca4127ccd8036cc743144ec21acd9b10..8db5e07308fdb48b4030a6cba8a1a54e53e8a48d 100644 (file)
--- a/runtime/src/main/java/org/aspectj/runtime/reflect/JoinPointImpl.java
+++ b/runtime/src/main/java/org/aspectj/runtime/reflect/JoinPointImpl.java
@@ -13,14 +13,15 @@
 
 package org.aspectj.runtime.reflect;
 
-import java.util.Stack;
-
 import org.aspectj.lang.JoinPoint;
 import org.aspectj.lang.ProceedingJoinPoint;
 import org.aspectj.lang.Signature;
 import org.aspectj.lang.reflect.SourceLocation;
 import org.aspectj.runtime.internal.AroundClosure;
 
+import java.util.ArrayList;
+import java.util.List;
+
 class JoinPointImpl implements ProceedingJoinPoint {
        static class StaticPartImpl implements JoinPoint.StaticPart {
                String kind;
@@ -79,18 +80,6 @@ class JoinPointImpl implements ProceedingJoinPoint {
                }
        }
 
-       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 target;
        Object[] args;
@@ -152,23 +141,26 @@ class JoinPointImpl implements ProceedingJoinPoint {
        // will either be using arc or arcs but not both. arcs being non-null
        // indicates it is in use (even if an empty stack)
        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) {
                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 (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 {
                // when called from a before advice, but be a no-op
@@ -179,19 +171,14 @@ class JoinPointImpl implements ProceedingJoinPoint {
                                return arc.run(arc.getState());
                        }
                } else {
-                       final AroundClosure ac = arcs.get().peek();
+                       final AroundClosure ac = arcs.get(arcIndex.get());
                        return ac.run(ac.getState());
                }
        }
 
        public Object proceed(Object[] adviceBindings) throws Throwable {
                // 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) {
                        return null;

diff --git a/tests/src/test/java/org/aspectj/systemtest/ajc199/Bugs199Tests.java b/tests/src/test/java/org/aspectj/systemtest/ajc199/Bugs199Tests.java
index 6ec1291d4d45867bedf41ba2bac4e4209257aec4..3428c3e3c9602416b404884ae6a4d8e2faf4f8a0 100644 (file)
--- a/tests/src/test/java/org/aspectj/systemtest/ajc199/Bugs199Tests.java
+++ b/tests/src/test/java/org/aspectj/systemtest/ajc199/Bugs199Tests.java
@@ -49,8 +49,9 @@ public class Bugs199Tests extends XMLBasedAjcTestCase {
   }
 
   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() {

diff --git a/tests/src/test/resources/org/aspectj/systemtest/ajc199/ajc199.xml b/tests/src/test/resources/org/aspectj/systemtest/ajc199/ajc199.xml
index d8868ca2240eb6628f000b55551b1b47495e485d..35136023a5ee3f39b60cd37f0d0bea82d71c3924 100644 (file)
--- a/tests/src/test/resources/org/aspectj/systemtest/ajc199/ajc199.xml
+++ b/tests/src/test/resources/org/aspectj/systemtest/ajc199/ajc199.xml
@@ -196,7 +196,7 @@
        </ajc-test>
 
   <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">
                        <stdout ordered="no">
                                <line text=">> Outer intercept"/>
@@ -275,7 +275,7 @@
        </ajc-test>
 
        <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">
                        <stdout ordered="no">
                                <line text=">> Outer intercept"/>
@@ -354,7 +354,7 @@
        </ajc-test>
 
        <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">
                        <stdout ordered="no">
                                <line text=">> Outer intercept"/>
@@ -433,7 +433,7 @@
        </ajc-test>
 
        <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">
                        <stdout ordered="no">
                                <line text=">> Outer intercept"/>