From 43df701a10a80306dd98da8644a9bbfbf5d17089 Mon Sep 17 00:00:00 2001 From: Alexander Kriegisch Date: Sat, 2 Mar 2024 11:52:31 +0100 Subject: [PATCH] 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 --- .../runtime/reflect/JoinPointImpl.java | 45 +++++++------------ .../systemtest/ajc199/Bugs199Tests.java | 5 ++- .../org/aspectj/systemtest/ajc199/ajc199.xml | 8 ++-- 3 files changed, 23 insertions(+), 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 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> { - @Override - protected Stack initialValue() { - return new Stack<>(); - } - - @Override - protected Stack childValue(Stack parentValue) { - return (Stack) 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 arcs = null; + private final ThreadLocal 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 6ec1291d4..3428c3e3c 100644 --- 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 d8868ca22..35136023a 100644 --- 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 @@ - + @@ -275,7 +275,7 @@ - + @@ -354,7 +354,7 @@ - + @@ -433,7 +433,7 @@ - + -- 2.39.5