diff --git a/org.aspectj.ajdt.core/src/test/java/org/aspectj/tools/ajc/AjcTestCase.java b/org.aspectj.ajdt.core/src/test/java/org/aspectj/tools/ajc/AjcTestCase.java index 11b90c3b73..b76fe4ee01 100644 --- a/org.aspectj.ajdt.core/src/test/java/org/aspectj/tools/ajc/AjcTestCase.java +++ b/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 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 851a8b3dca..8db5e07308 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; @@ -79,18 +80,6 @@ public EnclosingStaticPartImpl(int count, String kind, Signature signature, Sour } } - static class InheritableThreadLocalAroundClosureStack extends InheritableThreadLocal> { - @Override - protected Stack initialValue() { - return new Stack<>(); - } - - @Override - protected Stack childValue(Stack parentValue) { - return (Stack) 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 arcs = null; + private final ThreadLocal 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; diff --git a/tests/bugs1921/github_288/FirstAspect.aj b/tests/bugs1921/github_288/FirstAspect.aj new file mode 100644 index 0000000000..63ef73ff90 --- /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 0000000000..f81e90a552 --- /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 0000000000..54a5eb1cc6 --- /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 0000000000..ef405e4eeb --- /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 40459ba8dc..8395b5bbf6 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 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/java/org/aspectj/systemtest/ajc199/Bugs199Tests.java b/tests/src/test/java/org/aspectj/systemtest/ajc199/Bugs199Tests.java index 6ec1291d4d..3428c3e3c9 100644 --- a/tests/src/test/java/org/aspectj/systemtest/ajc199/Bugs199Tests.java +++ b/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() { 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 975fa1c6c6..494b6daddc 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 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/src/test/resources/org/aspectj/systemtest/ajc199/ajc199.xml b/tests/src/test/resources/org/aspectj/systemtest/ajc199/ajc199.xml index d8868ca224..35136023a5 100644 --- a/tests/src/test/resources/org/aspectj/systemtest/ajc199/ajc199.xml +++ b/tests/src/test/resources/org/aspectj/systemtest/ajc199/ajc199.xml @@ -196,7 +196,7 @@ - + @@ -275,7 +275,7 @@ - + @@ -354,7 +354,7 @@ - + @@ -433,7 +433,7 @@ - +