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 6, 2024
1 parent 6743f7c commit 4414b42
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);

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);

This comment has been minimized.

Sign in to view

Copy link
@pagrawalgit

pagrawalgit Mar 6, 2024

I am not very thorough with this code. I am wondering if there would ever be a scenario where this method could be executed simultaneously by two different threads and thereby possibility of a race condition?

Again, it may never happen, just curious.

This comment has been minimized.

Sign in to view

Copy link
@kriegaex

kriegaex Mar 7, 2024

Author Contributor

Yes, in theory it is possible. If this ever causes problems in the future, we can always add more guard rails and synchronise or lock on something. For now, this part of the code is no worse than it was for the last 5 or so years. Feel free to present a test case breaking the code, and I will take another look.

This comment has been minimized.

Sign in to view

Copy link
@pagrawalgit

pagrawalgit Mar 7, 2024

Thanks

This comment has been minimized.

Sign in to view

Copy link
@kriegaex

kriegaex Mar 8, 2024

Author Contributor

I made a quick experiment with two around advices, each proceeding twice, once in the same and once in a new thread. I ran 10,000 rounds of the woven method in a thread pool sized 10, counted the expected number of executions for both aspects (n for the outer one, 2n for the inner one) and woven method (4n) and could not make it fail. It looks pretty solid.

This comment has been minimized.

Sign in to view

Copy link
@pagrawalgit

pagrawalgit Mar 8, 2024

That's great!
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 4414b42

Please sign in to comment.