From a3e3680b1bafbfe86c115bf3d42b8e871954065a Mon Sep 17 00:00:00 2001 From: Alexander Kriegisch Date: Sat, 26 Feb 2022 10:08:21 +0700 Subject: [PATCH 1/3] Synchronise JoinPointImpl methods dealing with Stack If we do not synchronise, we will run into problems with nested around-aspects in combination with proceeding asynchronously (in another thread). Fixes #128. Signed-off-by: Alexander Kriegisch --- .../java/org/aspectj/runtime/reflect/JoinPointImpl.java | 6 +++--- 1 file changed, 3 insertions(+), 3 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 bb09e68697..0c2764613e 100644 --- a/runtime/src/main/java/org/aspectj/runtime/reflect/JoinPointImpl.java +++ b/runtime/src/main/java/org/aspectj/runtime/reflect/JoinPointImpl.java @@ -146,7 +146,7 @@ public final String toLongString() { this.arc = arc; } - public void stack$AroundClosure(AroundClosure arc) { + public synchronized void stack$AroundClosure(AroundClosure arc) { // If input parameter arc is null this is the 'unlink' call from AroundClosure if (arcs == null) { arcs = new Stack<>(); @@ -158,7 +158,7 @@ public final String toLongString() { } } - public Object proceed() throws Throwable { + public synchronized Object proceed() throws Throwable { // when called from a before advice, but be a no-op if (arcs == null) { if (arc == null) { @@ -171,7 +171,7 @@ public Object proceed() throws Throwable { } } - public Object proceed(Object[] adviceBindings) throws Throwable { + public synchronized Object proceed(Object[] adviceBindings) throws Throwable { // when called from a before advice, but be a no-op AroundClosure ac = null; if (arcs == null) { From 1584914314aa7ad69e08fa295984df784e204d1f Mon Sep 17 00:00:00 2001 From: Alexander Kriegisch Date: Sat, 26 Feb 2022 12:33:50 +0700 Subject: [PATCH 2/3] Never pop elements from Stack Otherwise, we would jeopardise asynchronous proceeding, see https://github.com/eclipse/org.aspectj/pull/129#issuecomment-1051609529 Instead, we - keep the stack, - but treat it like a list (Stack implements the List interface), - keeping a current closure index, - using 'get(currentArcIndex)' instead of `peek()` and `pop()`. Fixes #128. Signed-off-by: Alexander Kriegisch --- .../runtime/reflect/JoinPointImpl.java | 29 ++++++++++--------- 1 file changed, 16 insertions(+), 13 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 0c2764613e..0aae4be0dc 100644 --- a/runtime/src/main/java/org/aspectj/runtime/reflect/JoinPointImpl.java +++ b/runtime/src/main/java/org/aspectj/runtime/reflect/JoinPointImpl.java @@ -140,7 +140,8 @@ 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 Stack arcs = null; + private final Stack arcs = new Stack<>(); + private int currentArcIndex = -1; public void set$AroundClosure(AroundClosure arc) { this.arc = arc; @@ -148,36 +149,35 @@ public final String toLongString() { public synchronized void stack$AroundClosure(AroundClosure arc) { // If input parameter arc is null this is the 'unlink' call from AroundClosure - if (arcs == null) { - arcs = new Stack<>(); - } - if (arc==null) { - this.arcs.pop(); - } else { + if (arc != null) { this.arcs.push(arc); + currentArcIndex++; } - } + } public synchronized Object proceed() throws Throwable { // when called from a before advice, but be a no-op - if (arcs == null) { + if (currentArcIndex < 0) { if (arc == null) { return null; } else { return arc.run(arc.getState()); } } else { - return arcs.peek().run(arcs.peek().getState()); + final AroundClosure ac = arcs.get(currentArcIndex--); + final Object result = ac.run(ac.getState()); + currentArcIndex++; + return result; } } public synchronized Object proceed(Object[] adviceBindings) throws Throwable { // when called from a before advice, but be a no-op AroundClosure ac = null; - if (arcs == null) { + if (currentArcIndex < 0) { ac = arc; } else { - ac = arcs.peek(); + ac = arcs.get(currentArcIndex); } if (ac == null) { @@ -254,7 +254,10 @@ public synchronized Object proceed(Object[] adviceBindings) throws Throwable { // state[i] = adviceBindings[formalIndex]; // } // } - return ac.run(state); + currentArcIndex--; + final Object result = ac.run(state); + currentArcIndex++; + return result; } } From 436bf4dc00ca306371505d65a07b2cbe22a10021 Mon Sep 17 00:00:00 2001 From: Alexander Kriegisch Date: Sat, 26 Feb 2022 13:12:02 +0700 Subject: [PATCH 3/3] Clean up JoinPointImpl, e.g. use list instead of stack Fixes #128. Signed-off-by: Alexander Kriegisch --- .../runtime/reflect/JoinPointImpl.java | 29 +++++++------------ 1 file changed, 10 insertions(+), 19 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 0aae4be0dc..0e71478170 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; @@ -137,20 +138,19 @@ public final String toLongString() { } // To proceed we need a closure to proceed on. Generated code - // will either be using arc or arcs but not both. arcs being non-null - // indicates it is in use (even if an empty stack) + // will either be using arc or arcs but not both. private AroundClosure arc = null; - private final Stack arcs = new Stack<>(); - private int currentArcIndex = -1; + private final List arcs = new ArrayList<>(); + private int currentArcIndex = -1; public void set$AroundClosure(AroundClosure arc) { this.arc = arc; } - public synchronized void stack$AroundClosure(AroundClosure arc) { + public synchronized void stack$AroundClosure(AroundClosure arc) { // If input parameter arc is null this is the 'unlink' call from AroundClosure if (arc != null) { - this.arcs.push(arc); + this.arcs.add(arc); currentArcIndex++; } } @@ -158,11 +158,7 @@ public final String toLongString() { public synchronized Object proceed() throws Throwable { // when called from a before advice, but be a no-op if (currentArcIndex < 0) { - if (arc == null) { - return null; - } else { - return arc.run(arc.getState()); - } + return arc == null ? null : arc.run(arc.getState()); } else { final AroundClosure ac = arcs.get(currentArcIndex--); final Object result = ac.run(ac.getState()); @@ -173,12 +169,7 @@ public synchronized Object proceed() throws Throwable { public synchronized Object proceed(Object[] adviceBindings) throws Throwable { // when called from a before advice, but be a no-op - AroundClosure ac = null; - if (currentArcIndex < 0) { - ac = arc; - } else { - ac = arcs.get(currentArcIndex); - } + AroundClosure ac = currentArcIndex < 0 ? arc : arcs.get(currentArcIndex); if (ac == null) { return null;