open-telemetry
/
opentelemetry-java-instrumentation
mirror of https://github.com/open-telemetry/opentelemetry-java-instrumentation.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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.
This commit is contained in:
Nikolay Martynov 2018-12-05 14:48:07 -05:00
parent 30712cdc87
commit 4deb68bfd7
4 changed files with 13 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java
View File

@ -29,10 +29,6 @@ public interface WeakMap<K, V> {
} }
} }
public static boolean isProviderRegistered() {
return provider.get() != Supplier.DEFAULT;
}
public static <K, V> WeakMap<K, V> newWeakMap() { public static <K, V> WeakMap<K, V> newWeakMap() {
return provider.get().get(); return provider.get().get();
} }

8
dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java
View File

@ -111,11 +111,9 @@ public class AgentInstaller {
} }
private static void registerWeakMapProvider() { private static void registerWeakMapProvider() {
if (!WeakMap.Provider.isProviderRegistered()) { WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.WeakConcurrent());
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.Inline());
// WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.Guava());
} }
@Slf4j @Slf4j

12
dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java
View File

@ -54,7 +54,7 @@ class WeakMapSuppliers {
}; };
private final ScheduledExecutorService cleanerExecutorService; private final ScheduledExecutorService cleanerExecutorService;
private final Thread shutdownCallback;
private final Queue<WeakReference<WeakConcurrentMap>> suppliedMaps = private final Queue<WeakReference<WeakConcurrentMap>> suppliedMaps =
new ConcurrentLinkedQueue<>(); new ConcurrentLinkedQueue<>();
@ -62,14 +62,16 @@ class WeakMapSuppliers {
WeakConcurrent() { WeakConcurrent() {
cleanerExecutorService = Executors.newScheduledThreadPool(1, THREAD_FACTORY); cleanerExecutorService = Executors.newScheduledThreadPool(1, THREAD_FACTORY);
shutdownCallback = new ShutdownCallback(cleanerExecutorService);
cleanerExecutorService.scheduleAtFixedRate( cleanerExecutorService.scheduleAtFixedRate(
new CleanupRunnable(cleanerExecutorService, suppliedMaps, finalized), new CleanupRunnable(cleanerExecutorService, shutdownCallback, suppliedMaps, finalized),
CLEAN_FREQUENCY_SECONDS, CLEAN_FREQUENCY_SECONDS,
CLEAN_FREQUENCY_SECONDS, CLEAN_FREQUENCY_SECONDS,
TimeUnit.SECONDS); TimeUnit.SECONDS);
try { try {
Runtime.getRuntime().addShutdownHook(new ShutdownCallback(cleanerExecutorService)); Runtime.getRuntime().addShutdownHook(shutdownCallback);
} catch (final IllegalStateException ex) { } catch (final IllegalStateException ex) {
// The JVM is already shutting down. // The JVM is already shutting down.
} }
@ -90,14 +92,17 @@ class WeakMapSuppliers {
private static class CleanupRunnable implements Runnable { private static class CleanupRunnable implements Runnable {
private final ScheduledExecutorService executorService; private final ScheduledExecutorService executorService;
private final Thread shutdownCallback;
private final Queue<WeakReference<WeakConcurrentMap>> suppliedMaps; private final Queue<WeakReference<WeakConcurrentMap>> suppliedMaps;
private final AtomicBoolean finalized; private final AtomicBoolean finalized;
public CleanupRunnable( public CleanupRunnable(
final ScheduledExecutorService executorService, final ScheduledExecutorService executorService,
final Thread shutdownCallback,
final Queue<WeakReference<WeakConcurrentMap>> suppliedMaps, final Queue<WeakReference<WeakConcurrentMap>> suppliedMaps,
final AtomicBoolean finalized) { final AtomicBoolean finalized) {
this.executorService = executorService; this.executorService = executorService;
this.shutdownCallback = shutdownCallback;
this.suppliedMaps = suppliedMaps; this.suppliedMaps = suppliedMaps;
this.finalized = finalized; this.finalized = finalized;
} }
@ -115,6 +120,7 @@ class WeakMapSuppliers {
} }
if (finalized.get() && suppliedMaps.isEmpty()) { if (finalized.get() && suppliedMaps.isEmpty()) {
executorService.shutdown(); executorService.shutdown();
Runtime.getRuntime().removeShutdownHook(shutdownCallback);
} }
} }
} }

2
dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy
View File

@ -107,7 +107,7 @@ class WeakConcurrentSupplierTest extends Specification {
// Hit map a few times to trigger unreferenced entries cleanup. // Hit map a few times to trigger unreferenced entries cleanup.
// Exact number of times that we need to hit map is implementation dependent. // 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 // com.google.common.collect.MapMakerInternalMap.DRAIN_THRESHOLD = 0x3F
if (name == "Guava" || name == "WeakInline") { if (name == "Guava" || name == "WeakInline") {
for (int i = 0; i <= 0x3F; i++) { for (int i = 0; i <= 0x3F; i++) {