From 966397e3bfe0d38aeca28c6aec0f543d62a0e2ea Mon Sep 17 00:00:00 2001 From: Alexander Kriegisch Date: Wed, 6 Mar 2024 15:27:50 +0100 Subject: Add regression tests for inheritable thread-local memory leak Leak was introduced in commit 3c80a36527, 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 --- tests/bugs1921/github_288/FirstAspect.aj | 12 +++ tests/bugs1921/github_288/MemoryHog.java | 18 ++++ .../NestedAroundClosureMemoryLeakTest.java | 104 +++++++++++++++++++++ tests/bugs1921/github_288/SecondAspect.aj | 12 +++ .../aspectj/systemtest/ajc1921/Bugs1921Tests.java | 8 ++ .../org/aspectj/systemtest/ajc1921/ajc1921.xml | 52 +++++++++++ 6 files changed, 206 insertions(+) create mode 100644 tests/bugs1921/github_288/FirstAspect.aj create mode 100644 tests/bugs1921/github_288/MemoryHog.java create mode 100644 tests/bugs1921/github_288/NestedAroundClosureMemoryLeakTest.java create mode 100644 tests/bugs1921/github_288/SecondAspect.aj diff --git a/tests/bugs1921/github_288/FirstAspect.aj b/tests/bugs1921/github_288/FirstAspect.aj new file mode 100644 index 000000000..63ef73ff9 --- /dev/null +++ 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 new file mode 100644 index 000000000..f81e90a55 --- /dev/null +++ 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 new file mode 100644 index 000000000..54a5eb1cc --- /dev/null +++ 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 executorServices = createExecutorServicesWithFixedThreadPools(numThreadPools); + try { + executeTasksAndGC(executorServices); + + Field mapField = Thread.class.getDeclaredField("inheritableThreadLocals"); + mapField.setAccessible(true); + Set 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. + *

+ * If each thread allocates memory for a stack of around closures (memory leak case) according to + * GitHub issue 288, the program will run out of + * memory. When fixed, this should no longer happen. + *

+ * 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 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 createExecutorServicesWithFixedThreadPools(int count) { + List executorServiceList = new ArrayList<>(count); + for (int i = 0; i < count; i++) + executorServiceList.add(Executors.newFixedThreadPool(1)); + return executorServiceList; + } + + private static void executeTasksAndGC(List 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 new file mode 100644 index 000000000..ef405e4ee --- /dev/null +++ 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 index 40459ba8d..8395b5bbf 100644 --- 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 class Bugs1921Tests extends XMLBasedAjcTestCase { 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 index 975fa1c6c..494b6dadd 100644 --- 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 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + -- cgit v1.2.3