From 43df701a10a80306dd98da8644a9bbfbf5d17089 Mon Sep 17 00:00:00 2001
From: Alexander Kriegisch <Alexander@Kriegisch.name>
Date: Sat, 2 Mar 2024 11:52:31 +0100
Subject: 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>
---
 .../org/aspectj/runtime/reflect/JoinPointImpl.java | 45 ++++++++--------------
 1 file changed, 16 insertions(+), 29 deletions(-)

(limited to 'runtime/src/main')

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 851a8b3dc..8db5e0730 100644
--- 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;
-- 
cgit v1.2.3