Avoid ThreadLocal memory leak in JoinPointImpl

org.aspectj.ajdt.core/src/test/java/org/aspectj/tools/ajc/AjcTestCase.java

@@ -694,10 +694,10 @@ else if (moduleName != null) {
 		else if (
 			vmargs != null && (
 				vmargs.contains("--enable-preview") ||
-				vmargs.contains("--add-modules") ||
-				vmargs.contains("--limit-modules") ||
-				vmargs.contains("--add-reads") ||
-				vmargs.contains("--add-exports")
+					vmargs.contains("--add-modules") || vmargs.contains("--limit-modules") ||
+					vmargs.contains("--add-reads") || vmargs.contains("--add-exports") || vmargs.contains("--add-opens") ||
+					vmargs.contains("-ea") || vmargs.contains("-enableassertions") ||
+					vmargs.contains("-Xmx")
 			)
 		) {
 			// If --add-modules supplied, need to fork the test
@@ -708,7 +708,10 @@ else if (
 				if (cp.indexOf("aspectjrt")==-1) {
 					cp.append(pathSeparator).append(TestUtil.aspectjrtPath().getPath());
 				}
-				String command = LangUtil.getJavaExecutable().getAbsolutePath() + " " +vmargs+ (cp.length()==0?"":" -classpath " + cp) + " " + className   ;
+				String command = LangUtil.getJavaExecutable().getAbsolutePath() + " " +
+					vmargs +
+					(cp.length() == 0 ? "" : " -classpath " + cp) + " " +
+					className + " " + String.join(" ", args);
 				if (Ajc.verbose)
 					System.out.println("\nCommand: '" + command + "'\n");
 				// Command is executed using ProcessBuilder to allow setting CWD for ajc sandbox compliance

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

tests/bugs1921/github_288/FirstAspect.aj

@@ -0,0 +1,12 @@
+import org.aspectj.lang.ProceedingJoinPoint;
+import org.aspectj.lang.annotation.Around;
+import org.aspectj.lang.annotation.Aspect;
+
+@Aspect
+public class FirstAspect {
+  @Around("execution(* doSomething())")
+  public Object doAround(ProceedingJoinPoint joinPoint) throws Throwable {
+    System.out.println(getClass().getSimpleName());
+    return joinPoint.proceed();
+  }
+}

tests/bugs1921/github_288/MemoryHog.java

@@ -0,0 +1,18 @@
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+
+public class MemoryHog {
+  final ExecutorService taskManager;
+  // Use 128 MB of data, then run with -Xmx1G for 10 threads or -Xmx512M for 5 threads
+  final byte[] someBigData = new byte[1024 * 1024 * 128];
+
+  public MemoryHog(final ExecutorService executorService) {
+    taskManager = executorService;
+  }
+
+  public void doSomething() throws ExecutionException, InterruptedException {
+    Future<?> future = taskManager.submit(() -> System.out.println("Executing task"));
+    future.get();
+  }
+}

tests/bugs1921/github_288/NestedAroundClosureMemoryLeakTest.java

@@ -0,0 +1,104 @@
+import java.lang.reflect.Field;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+
+public class NestedAroundClosureMemoryLeakTest {
+
+  public static void main(String[] args) throws Exception {
+    if (args.length > 0 && "oom".equals(args[0]))
+      testNoMemoryLeak_SystemShouldNotRunOutOfMemory();
+    else
+      testNoMemoryLeak_InheritableThreadLocalCleared();
+  }
+
+  /**
+   * Tests that the inheritable thread-locals of the spawned threads are either null or contain all null elements
+   */
+  public static void testNoMemoryLeak_InheritableThreadLocalCleared() throws Exception {
+    int numThreadPools = 1;
+    List<ExecutorService> executorServices = createExecutorServicesWithFixedThreadPools(numThreadPools);
+    try {
+      executeTasksAndGC(executorServices);
+
+      Field mapField = Thread.class.getDeclaredField("inheritableThreadLocals");
+      mapField.setAccessible(true);
+      Set<Thread> threads = Thread.getAllStackTraces().keySet();
+
+      threads.stream()
+        .filter(thread -> thread.getName().contains("pool"))
+        .forEach(thread -> {
+          try {
+            Object inheritableThreadLocals = mapField.get(thread);
+            if (inheritableThreadLocals != null) {
+              Field tableField = inheritableThreadLocals.getClass().getDeclaredField("table");
+              tableField.setAccessible(true);
+              Object[] inheritableThreadLocalTable = (Object[]) tableField.get(inheritableThreadLocals);
+              if (inheritableThreadLocalTable != null) {
+                for (Object entry : inheritableThreadLocalTable)
+                  assert entry == null : "All inheritable thread-locals should be null after GC";
+              }
+            }
+          }
+          catch (Exception e) {
+            throw new RuntimeException(e);
+          }
+        });
+
+      System.out.println("Test passed - all inheritable thread-locals are null after GC");
+    }
+    finally {
+      for (ExecutorService executorService : executorServices)
+        executorService.shutdown();
+    }
+  }
+
+  /**
+   * Executes tasks in multiple threads, using one executor service with a fixed thread pool of size 1 per task. This
+   * ensures that each spawned thread gets initialised for the first time and allocates memory for inheritable
+   * thread-locals, exposing possible memory leaks when running @AspectJ aspects with non-inlined, nested around advices
+   * in multi-thread situations.
+   * <p>
+   * If each thread allocates memory for a stack of around closures (memory leak case) according to
+   * <a href="https://github.com/eclipse-aspectj/aspectj/issues/288">GitHub issue 288</a>, the program will run out of
+   * memory. When fixed, this should no longer happen.
+   * <p>
+   * Run this test e.g. with {@code -Xmx1G} for 10 x 128 MB memory consumption to ensure an out of memory error in the
+   * leak case. Any other appropriate combination of number of threads and memory limit is also OK.
+   */
+  public static void testNoMemoryLeak_SystemShouldNotRunOutOfMemory() throws Exception {
+    int numThreadPools = 5;
+    List<ExecutorService> executorServices = createExecutorServicesWithFixedThreadPools(numThreadPools);
+    try {
+      executeTasksAndGC(executorServices);
+      System.out.println("Test passed - no OutOfMemoryError due to inheritable thread-locals memory leak");
+    }
+    finally {
+      for (ExecutorService executorService : executorServices)
+        executorService.shutdown();
+    }
+  }
+
+  private static List<ExecutorService> createExecutorServicesWithFixedThreadPools(int count) {
+    List<ExecutorService> executorServiceList = new ArrayList<>(count);
+    for (int i = 0; i < count; i++)
+      executorServiceList.add(Executors.newFixedThreadPool(1));
+    return executorServiceList;
+  }
+
+  private static void executeTasksAndGC(List<ExecutorService> executorServices) throws Exception {
+    for (ExecutorService executorService : executorServices)
+      new MemoryHog(executorService).doSomething();
+    System.out.println("Finished executing tasks");
+
+    // Best effort GC
+    System.gc();
+    System.out.println("Finished executing GC");
+
+    // Sleep to take a memory dump
+    // Thread.sleep(500000);
+  }
+
+}

tests/bugs1921/github_288/SecondAspect.aj

@@ -0,0 +1,12 @@
+import org.aspectj.lang.ProceedingJoinPoint;
+import org.aspectj.lang.annotation.Around;
+import org.aspectj.lang.annotation.Aspect;
+
+@Aspect
+public class SecondAspect {
+  @Around("execution(* doSomething())")
+  public Object doAround(ProceedingJoinPoint joinPoint) throws Throwable {
+    System.out.println(getClass().getSimpleName());
+    return joinPoint.proceed();
+  }
+}

tests/src/test/java/org/aspectj/systemtest/ajc1921/Bugs1921Tests.java

@@ -23,6 +23,14 @@ public void testGitHub_285() {
     runTest("shared cache negative test");
   }
 
+  public void testGitHub_288_AssertionError() {
+    runTest("memory leak for @AspectJ nested, non-inlined around-advice - AssertionError");
+  }
+
+  public void testGitHub_288_OutOfMemoryError() {
+    runTest("memory leak for @AspectJ nested, non-inlined around-advice - OutOfMemoryError");
+  }
+
   public static Test suite() {
     return XMLBasedAjcTestCase.loadSuite(Bugs1921Tests.class);
   }

tests/src/test/java/org/aspectj/systemtest/ajc199/Bugs199Tests.java

@@ -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() {

tests/src/test/resources/org/aspectj/systemtest/ajc1921/ajc1921.xml

@@ -393,4 +393,56 @@
 		</run>
 	</ajc-test>
 
+	<!--
+		https://github.com/eclipse-aspectj/aspectj/issues/288,
+		https://github.com/eclipse-aspectj/aspectj/issues/141, AspectJ 1.9.21.2
+	-->
+	<ajc-test dir="bugs1921/github_288" title="memory leak for @AspectJ nested, non-inlined around-advice - AssertionError">
+		<compile files="NestedAroundClosureMemoryLeakTest.java MemoryHog.java FirstAspect.aj SecondAspect.aj" options="-1.8 -XnoInline"/>
+		<run class="NestedAroundClosureMemoryLeakTest" vmargs="-ea --add-opens java.base/java.lang=ALL-UNNAMED">
+			<stdout>
+				<line text="FirstAspect"/>
+				<line text="SecondAspect"/>
+				<line text="Executing task"/>
+				<line text="Finished executing tasks"/>
+				<line text="Finished executing GC"/>
+				<line text="Test passed - all inheritable thread-locals are null after GC"/>
+			</stdout>
+			<!-- No AssertionError on stderr-->
+			<stderr/>
+		</run>
+	</ajc-test>
+
+	<!--
+		https://github.com/eclipse-aspectj/aspectj/issues/288,
+		https://github.com/eclipse-aspectj/aspectj/issues/141, AspectJ 1.9.21.2
+	-->
+	<ajc-test dir="bugs1921/github_288" title="memory leak for @AspectJ nested, non-inlined around-advice - OutOfMemoryError">
+		<compile files="NestedAroundClosureMemoryLeakTest.java MemoryHog.java FirstAspect.aj SecondAspect.aj" options="-1.8 -XnoInline"/>
+		<run class="NestedAroundClosureMemoryLeakTest" vmargs="-ea --add-opens java.base/java.lang=ALL-UNNAMED -Xmx512M" options="oom">
+			<stdout>
+				<line text="FirstAspect"/>
+				<line text="SecondAspect"/>
+				<line text="Executing task"/>
+				<line text="FirstAspect"/>
+				<line text="SecondAspect"/>
+				<line text="Executing task"/>
+				<line text="FirstAspect"/>
+				<line text="SecondAspect"/>
+				<line text="Executing task"/>
+				<line text="FirstAspect"/>
+				<line text="SecondAspect"/>
+				<line text="Executing task"/>
+				<line text="FirstAspect"/>
+				<line text="SecondAspect"/>
+				<line text="Executing task"/>
+				<line text="Finished executing tasks"/>
+				<line text="Finished executing GC"/>
+				<line text="Test passed - no OutOfMemoryError due to inheritable thread-locals memory leak"/>
+			</stdout>
+			<!-- No fatal OutOfMemoryError on stderr -->
+			<stderr/>
+		</run>
+	</ajc-test>
+
 </suite>

tests/src/test/resources/org/aspectj/systemtest/ajc199/ajc199.xml

@@ -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"/>
@@ -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"/>
@@ -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"/>
@@ -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"/>