Add regression tests for inheritable thread-local memory leak

Leak was introduced in commit 3c80a36, fixing #128, but introducing #288 instead, which was the lesser of two evils, but still bad for some users unwilling to use native AspectJ syntax for their aspects, avoiding the problem. Relates to #288. Signed-off-by: Alexander Kriegisch <[email protected]>
eclipse-aspectj · Mar 6, 2024 · 6743f7c · 6743f7c
1 parent 109d9cb
commit 6743f7c
Show file tree

Hide file tree

Showing 6 changed files with 206 additions and 0 deletions.
diff --git a/tests/bugs1921/github_288/FirstAspect.aj b/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();
+  }
+}
diff --git a/tests/bugs1921/github_288/MemoryHog.java b/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();
+  }
+}
diff --git a/tests/bugs1921/github_288/NestedAroundClosureMemoryLeakTest.java b/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);
+  }
+
+}
diff --git a/tests/bugs1921/github_288/SecondAspect.aj b/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();
+  }
+}
diff --git a/tests/src/test/java/org/aspectj/systemtest/ajc1921/Bugs1921Tests.java b/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);
   }

diff --git a/tests/src/test/resources/org/aspectj/systemtest/ajc1921/ajc1921.xml b/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>