Skip to content

Commit

Permalink
Avoid ThreadLocal memory leak in JoinPointImpl
Browse files Browse the repository at this point in the history 
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]>
  • Loading branch information
@kriegaex
kriegaex committed Mar 12, 2024
1 parent 966397e commit 43df701
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 35 deletions.
45 changes: 16 additions & 29 deletions runtime/src/main/java/org/aspectj/runtime/reflect/JoinPointImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);

This comment has been minimized.

Sign in to view

Copy link
@KimmingLau

KimmingLau Apr 2, 2024

Contributor

Hi, I think here is a problem that after JoinPointImpl object was clean up by GC, the entry of ThreadLocalMap will not be released in time, because entry is WeakReference. Shall we consider proactive removing the ThreadLocal of arcIndex?

This comment has been minimized.

Sign in to view

Copy link
@KimmingLau

KimmingLau Apr 2, 2024

Contributor

Although ThreadLocalMap has a random clean-up mechanism for null key, it is possible causing a large amount of remaining entries to accumulate in case of calling the enhanced method under high traffic.

This comment has been minimized.

Sign in to view

Copy link
@kriegaex

kriegaex Apr 2, 2024

Author Contributor

@KimmingLau, the commit is 3 weeks old and is part of releases 1.9.21.2 and 1.9.22 already. Next time, quicker feedback would be good.

Question back: Do you have any concrete problems, or is this a theoretical thought? The perfect solution in this case does not exist anyway, other than using native syntax. If you have a real, reproducible problem, feel free to open an issue for it, even though I cannot promise to be able to fix it. If you have a concrete idea how to improve the code, please let me know.

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
Expand All @@ -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;
Expand Down
5 changes: 3 additions & 2 deletions tests/src/test/java/org/aspectj/systemtest/ajc199/Bugs199Tests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,9 @@ 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)");
// 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() {
Expand Down
8 changes: 4 additions & 4 deletions tests/src/test/resources/org/aspectj/systemtest/ajc199/ajc199.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"/>
Expand Down Expand Up @@ -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"/>
Expand Down Expand Up @@ -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"/>
Expand Down Expand Up @@ -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"/>
Expand Down

0 comments on commit 43df701

Please sign in to comment.