From 4deb68bfd76bc7d910ca9aa0c75888295edacff6 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Wed, 5 Dec 2018 14:48:07 -0500 Subject: [PATCH 1/4] Fix memory leak in WeakMapSuppliers.WeakConcurrent The problem there is that it references JVM shutdown hook that keeps reference to cleanup executor which potentially can keep references to all sort of things - and this doesn't get cleaned up untill JVM shutdown. Solution is to remove shutdown hook when supplier is being GCed. --- .../main/java/datadog/trace/bootstrap/WeakMap.java | 4 ---- .../datadog/trace/agent/tooling/AgentInstaller.java | 8 +++----- .../trace/agent/tooling/WeakMapSuppliers.java | 12 +++++++++--- .../agent/tooling/WeakConcurrentSupplierTest.groovy | 2 +- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java index 778426e2a5..b49da78791 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java @@ -29,10 +29,6 @@ public interface WeakMap { } } - public static boolean isProviderRegistered() { - return provider.get() != Supplier.DEFAULT; - } - public static WeakMap newWeakMap() { return provider.get().get(); } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java index 0c68905aee..1e84ffd3b2 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java @@ -111,11 +111,9 @@ public class AgentInstaller { } private static void registerWeakMapProvider() { - if (!WeakMap.Provider.isProviderRegistered()) { - WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.WeakConcurrent()); - } - // WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.WeakConcurrent.Inline()); - // WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.Guava()); + WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.WeakConcurrent()); + // WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.WeakConcurrent.Inline()); + // WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.Guava()); } @Slf4j diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java index 679fde502a..daa050c130 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java @@ -54,7 +54,7 @@ class WeakMapSuppliers { }; private final ScheduledExecutorService cleanerExecutorService; - + private final Thread shutdownCallback; private final Queue> suppliedMaps = new ConcurrentLinkedQueue<>(); @@ -62,14 +62,16 @@ class WeakMapSuppliers { WeakConcurrent() { cleanerExecutorService = Executors.newScheduledThreadPool(1, THREAD_FACTORY); + shutdownCallback = new ShutdownCallback(cleanerExecutorService); + cleanerExecutorService.scheduleAtFixedRate( - new CleanupRunnable(cleanerExecutorService, suppliedMaps, finalized), + new CleanupRunnable(cleanerExecutorService, shutdownCallback, suppliedMaps, finalized), CLEAN_FREQUENCY_SECONDS, CLEAN_FREQUENCY_SECONDS, TimeUnit.SECONDS); try { - Runtime.getRuntime().addShutdownHook(new ShutdownCallback(cleanerExecutorService)); + Runtime.getRuntime().addShutdownHook(shutdownCallback); } catch (final IllegalStateException ex) { // The JVM is already shutting down. } @@ -90,14 +92,17 @@ class WeakMapSuppliers { private static class CleanupRunnable implements Runnable { private final ScheduledExecutorService executorService; + private final Thread shutdownCallback; private final Queue> suppliedMaps; private final AtomicBoolean finalized; public CleanupRunnable( final ScheduledExecutorService executorService, + final Thread shutdownCallback, final Queue> suppliedMaps, final AtomicBoolean finalized) { this.executorService = executorService; + this.shutdownCallback = shutdownCallback; this.suppliedMaps = suppliedMaps; this.finalized = finalized; } @@ -115,6 +120,7 @@ class WeakMapSuppliers { } if (finalized.get() && suppliedMaps.isEmpty()) { executorService.shutdown(); + Runtime.getRuntime().removeShutdownHook(shutdownCallback); } } } diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy index 6c3e37403c..997347ea4e 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy @@ -107,7 +107,7 @@ class WeakConcurrentSupplierTest extends Specification { // Hit map a few times to trigger unreferenced entries cleanup. // Exact number of times that we need to hit map is implementation dependent. - // For Guava it i specified in + // For Guava it is specified in // com.google.common.collect.MapMakerInternalMap.DRAIN_THRESHOLD = 0x3F if (name == "Guava" || name == "WeakInline") { for (int i = 0; i <= 0x3F; i++) { From 0f4e0f40e4b6080df7a1082f63a44c57cefe671f Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Wed, 5 Dec 2018 14:52:02 -0500 Subject: [PATCH 2/4] Remove muzzle plugin check for dd clean up threads This was needed to make sure that we are not leaving threads behind. Threads were GCed before - but root cause was GC of executor itself (see previous commit). Now threads may stay between muzzle tasks if GC doesn't need to run yes so this check actually creates false positives. --- buildSrc/src/main/groovy/MuzzlePlugin.groovy | 7 ------- 1 file changed, 7 deletions(-) diff --git a/buildSrc/src/main/groovy/MuzzlePlugin.groovy b/buildSrc/src/main/groovy/MuzzlePlugin.groovy index 6ba5c9ac08..4ef740d69d 100644 --- a/buildSrc/src/main/groovy/MuzzlePlugin.groovy +++ b/buildSrc/src/main/groovy/MuzzlePlugin.groovy @@ -15,7 +15,6 @@ import org.eclipse.aether.spi.connector.transport.TransporterFactory import org.eclipse.aether.transport.http.HttpTransporterFactory import org.eclipse.aether.version.Version import org.gradle.api.Action -import org.gradle.api.GradleException import org.gradle.api.Plugin import org.gradle.api.Project import org.gradle.api.Task @@ -279,12 +278,6 @@ class MuzzlePlugin implements Plugin { Method assertionMethod = instrumentationCL.loadClass('datadog.trace.agent.tooling.muzzle.MuzzleVersionScanPlugin') .getMethod('assertInstrumentationMuzzled', ClassLoader.class, ClassLoader.class, boolean.class) assertionMethod.invoke(null, instrumentationCL, userCL, muzzleDirective.assertPass) - - for (Thread thread : Thread.getThreads()) { - if (thread.getName().startsWith("dd-")) { - throw new GradleException("Task $taskName has spawned a thread: $thread. This will prevent GC of dynamic muzzle classes. Aborting muzzle run.") - } - } } } runAfter.finalizedBy(muzzleTask) From 7728f9918e1e6975306fec3648078a86d51754ee Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Wed, 5 Dec 2018 14:53:57 -0500 Subject: [PATCH 3/4] Remove shutdown hook when DDTracer is shutdown This is similar to the problem with weak map supplier and may lead to a memory leak if we created/estroy many DDTracer objects. --- .../java/datadog/opentracing/DDTracer.java | 27 +++++++++++++------ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java b/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java index da28a2bf55..98008afca6 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java @@ -60,6 +60,11 @@ public class DDTracer implements io.opentracing.Tracer, Closeable, datadog.trace private final Map defaultSpanTags; /** A configured mapping of service names to update with new values */ private final Map serviceNameMappings; + /** + * JVM shutdown callback, keeping a reference to it to remove this if DDTracer gets destroyed + * earlier + */ + private final Thread shutdownCallback; /** Span context decorators */ private final Map> spanContextDecorators = @@ -163,15 +168,15 @@ public class DDTracer implements io.opentracing.Tracer, Closeable, datadog.trace this.runtimeId = runtimeId; this.serviceNameMappings = serviceNameMappings; + shutdownCallback = + new Thread() { + @Override + public void run() { + close(); + } + }; try { - Runtime.getRuntime() - .addShutdownHook( - new Thread() { - @Override - public void run() { - close(); - } - }); + Runtime.getRuntime().addShutdownHook(shutdownCallback); } catch (final IllegalStateException ex) { // The JVM is already shutting down. } @@ -200,6 +205,12 @@ public class DDTracer implements io.opentracing.Tracer, Closeable, datadog.trace log.info("New instance: {}", this); } + @Override + public void finalize() { + Runtime.getRuntime().removeShutdownHook(shutdownCallback); + shutdownCallback.run(); + } + /** * Returns the list of span context decorators * From 600674b03ad4189234f4a9a140ab1226bee1e354 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Wed, 5 Dec 2018 17:20:50 -0500 Subject: [PATCH 4/4] Put back muzzle check for 'dd-' threads --- buildSrc/src/main/groovy/MuzzlePlugin.groovy | 7 +++++++ .../src/main/java/datadog/trace/bootstrap/WeakMap.java | 4 ++++ .../java/datadog/trace/agent/tooling/AgentInstaller.java | 8 +++++--- 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/buildSrc/src/main/groovy/MuzzlePlugin.groovy b/buildSrc/src/main/groovy/MuzzlePlugin.groovy index 4ef740d69d..6ba5c9ac08 100644 --- a/buildSrc/src/main/groovy/MuzzlePlugin.groovy +++ b/buildSrc/src/main/groovy/MuzzlePlugin.groovy @@ -15,6 +15,7 @@ import org.eclipse.aether.spi.connector.transport.TransporterFactory import org.eclipse.aether.transport.http.HttpTransporterFactory import org.eclipse.aether.version.Version import org.gradle.api.Action +import org.gradle.api.GradleException import org.gradle.api.Plugin import org.gradle.api.Project import org.gradle.api.Task @@ -278,6 +279,12 @@ class MuzzlePlugin implements Plugin { Method assertionMethod = instrumentationCL.loadClass('datadog.trace.agent.tooling.muzzle.MuzzleVersionScanPlugin') .getMethod('assertInstrumentationMuzzled', ClassLoader.class, ClassLoader.class, boolean.class) assertionMethod.invoke(null, instrumentationCL, userCL, muzzleDirective.assertPass) + + for (Thread thread : Thread.getThreads()) { + if (thread.getName().startsWith("dd-")) { + throw new GradleException("Task $taskName has spawned a thread: $thread. This will prevent GC of dynamic muzzle classes. Aborting muzzle run.") + } + } } } runAfter.finalizedBy(muzzleTask) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java index b49da78791..778426e2a5 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java @@ -29,6 +29,10 @@ public interface WeakMap { } } + public static boolean isProviderRegistered() { + return provider.get() != Supplier.DEFAULT; + } + public static WeakMap newWeakMap() { return provider.get().get(); } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java index 1e84ffd3b2..0c68905aee 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java @@ -111,9 +111,11 @@ public class AgentInstaller { } private static void registerWeakMapProvider() { - WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.WeakConcurrent()); - // WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.WeakConcurrent.Inline()); - // WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.Guava()); + if (!WeakMap.Provider.isProviderRegistered()) { + WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.WeakConcurrent()); + } + // WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.WeakConcurrent.Inline()); + // WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.Guava()); } @Slf4j