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 <[email protected]>
eclipse-aspectj · Mar 6, 2024 · c3c2907 · c3c2907
1 parent 1f1d429
commit c3c2907
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 35 deletions.
diff --git 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 @@ public EnclosingStaticPartImpl(int count, String kind, Signature signature, Sour
 		}
 	}
 
-	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 @@ public final String toLongString() {
 	// 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 @@ public Object proceed() throws Throwable {
 				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
@@ -49,8 +49,7 @@ public void testAsyncProceedNestedAroundAdvice_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)");
+     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
@@ -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"/>