From c95baef487b82b027cc2b16b6b4b29e9a80f124a Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Mon, 20 Aug 2018 23:42:28 -0400 Subject: [PATCH] Wait for given reference to get GCed in `waitGC` This should help to make sure that object we are interested in actually gets GCed. This change also improves tests for PendingTrace and ScopeManager to make sure that trace gets cleaned up. --- .../integration/classloading/ClassLoadingTest.groovy | 2 +- .../trace/agent/test/IntegrationTestUtils.java | 8 ++++++-- .../agent/tooling/WeakConcurrentSupplierTest.groovy | 6 ++++-- .../main/java/datadog/trace/agent/test/TestUtils.java | 6 +++++- .../src/test/java/muzzle/MuzzleWeakReferenceTest.java | 8 ++++---- .../datadog/opentracing/PendingTraceTest.groovy | 6 ++++-- .../opentracing/scopemanager/ScopeManagerTest.groovy | 11 ++++++----- 7 files changed, 30 insertions(+), 17 deletions(-) diff --git a/dd-java-agent-ittests/src/test/groovy/datadog/trace/agent/integration/classloading/ClassLoadingTest.groovy b/dd-java-agent-ittests/src/test/groovy/datadog/trace/agent/integration/classloading/ClassLoadingTest.groovy index bfb04736f8..46b03e77f1 100644 --- a/dd-java-agent-ittests/src/test/groovy/datadog/trace/agent/integration/classloading/ClassLoadingTest.groovy +++ b/dd-java-agent-ittests/src/test/groovy/datadog/trace/agent/integration/classloading/ClassLoadingTest.groovy @@ -39,7 +39,7 @@ class ClassLoadingTest extends Specification { loader.loadClass(ClassToInstrument.getName()) loader = null - IntegrationTestUtils.awaitGC() + IntegrationTestUtils.awaitGC(ref) then: null == ref.get() diff --git a/dd-java-agent-ittests/src/test/java/datadog/trace/agent/test/IntegrationTestUtils.java b/dd-java-agent-ittests/src/test/java/datadog/trace/agent/test/IntegrationTestUtils.java index 34dcaa8b42..e7d83385a7 100644 --- a/dd-java-agent-ittests/src/test/java/datadog/trace/agent/test/IntegrationTestUtils.java +++ b/dd-java-agent-ittests/src/test/java/datadog/trace/agent/test/IntegrationTestUtils.java @@ -159,12 +159,16 @@ public class IntegrationTestUtils { } public static void awaitGC() { - System.gc(); // For good measure. Object obj = new Object(); - final WeakReference ref = new WeakReference<>(obj); + final WeakReference ref = new WeakReference<>(obj); obj = null; + awaitGC(ref); + } + + public static void awaitGC(final WeakReference ref) { while (ref.get() != null) { System.gc(); + System.runFinalization(); } } } 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 a8245c5570..a79002b492 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 @@ -45,8 +45,9 @@ class WeakConcurrentSupplierTest extends Specification { def ref = new WeakReference(map) when: + def mapRef = new WeakReference<>(map) map = null - TestUtils.awaitGC() + TestUtils.awaitGC(mapRef) then: ref.get() == null @@ -68,8 +69,9 @@ class WeakConcurrentSupplierTest extends Specification { map.size() == 1 when: + def keyRef = new WeakReference<>(key) key = null - TestUtils.awaitGC() + TestUtils.awaitGC(keyRef) if (name == "WeakConcurrent") { // Sleep enough time for cleanup thread to get scheduled. diff --git a/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/TestUtils.java b/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/TestUtils.java index 70ba64bad0..af8be1dabe 100644 --- a/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/TestUtils.java +++ b/dd-java-agent/testing/src/main/java/datadog/trace/agent/test/TestUtils.java @@ -208,8 +208,12 @@ public class TestUtils { public static void awaitGC() { Object obj = new Object(); - final WeakReference ref = new WeakReference<>(obj); + final WeakReference ref = new WeakReference<>(obj); obj = null; + awaitGC(ref); + } + + public static void awaitGC(final WeakReference ref) { while (ref.get() != null) { System.gc(); System.runFinalization(); diff --git a/dd-java-agent/testing/src/test/java/muzzle/MuzzleWeakReferenceTest.java b/dd-java-agent/testing/src/test/java/muzzle/MuzzleWeakReferenceTest.java index 2fe00dfa31..9b3af88423 100644 --- a/dd-java-agent/testing/src/test/java/muzzle/MuzzleWeakReferenceTest.java +++ b/dd-java-agent/testing/src/test/java/muzzle/MuzzleWeakReferenceTest.java @@ -16,17 +16,17 @@ public class MuzzleWeakReferenceTest { */ public static boolean classLoaderRefIsGarbageCollected() { ClassLoader loader = new URLClassLoader(new URL[0], null); - WeakReference clRef = new WeakReference<>(loader); - Reference[] refs = + final WeakReference clRef = new WeakReference<>(loader); + final Reference[] refs = ReferenceCreator.createReferencesFrom( TestClasses.MethodBodyAdvice.class.getName(), MuzzleWeakReferenceTest.class.getClassLoader()) .values() .toArray(new Reference[0]); - ReferenceMatcher refMatcher = new ReferenceMatcher(refs); + final ReferenceMatcher refMatcher = new ReferenceMatcher(refs); refMatcher.getMismatchedReferenceSources(loader); loader = null; - TestUtils.awaitGC(); + TestUtils.awaitGC(clRef); return clRef.get() == null; } } diff --git a/dd-trace-ot/src/test/groovy/datadog/opentracing/PendingTraceTest.groovy b/dd-trace-ot/src/test/groovy/datadog/opentracing/PendingTraceTest.groovy index 5e3de902a0..9104ab8335 100644 --- a/dd-trace-ot/src/test/groovy/datadog/opentracing/PendingTraceTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/opentracing/PendingTraceTest.groovy @@ -5,6 +5,7 @@ import datadog.trace.common.writer.ListWriter import spock.lang.Specification import spock.lang.Subject +import java.lang.ref.WeakReference import java.util.concurrent.TimeUnit class PendingTraceTest extends Specification { @@ -105,9 +106,10 @@ class PendingTraceTest extends Specification { writer == [] when: + def childRef = new WeakReference<>(child) child = null - while (!trace.clean()) { - TestUtils.awaitGC() + TestUtils.awaitGC(childRef) + while (trace.clean()) { } then: diff --git a/dd-trace-ot/src/test/groovy/datadog/opentracing/scopemanager/ScopeManagerTest.groovy b/dd-trace-ot/src/test/groovy/datadog/opentracing/scopemanager/ScopeManagerTest.groovy index 170aee79ce..09b7f7a8e3 100644 --- a/dd-trace-ot/src/test/groovy/datadog/opentracing/scopemanager/ScopeManagerTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/opentracing/scopemanager/ScopeManagerTest.groovy @@ -11,6 +11,7 @@ import io.opentracing.noop.NoopSpan import spock.lang.Specification import spock.lang.Subject +import java.lang.ref.WeakReference import java.util.concurrent.atomic.AtomicBoolean import java.util.concurrent.atomic.AtomicReference @@ -149,8 +150,9 @@ class ScopeManagerTest extends Specification { continuation.activate() if (forceGC) { continuation = null // Continuation references also hold up traces. - TestUtils.awaitGC() - ((DDSpanContext) scope.span().context()).trace.clean() + TestUtils.awaitGC() // The goal here is to make sure that continuation DOES NOT get GCed + while (((DDSpanContext) scope.span().context()).trace.clean()) { + } } if (autoClose) { if (continuation != null) { @@ -194,10 +196,9 @@ class ScopeManagerTest extends Specification { when: if (forceGC) { + def continuationRef = new WeakReference<>(continuation) continuation = null // Continuation references also hold up traces. - while (!((DDSpanContext) span.context()).trace.clean()) { - TestUtils.awaitGC() - } + TestUtils.awaitGC(continuationRef) writer.waitForTraces(1) } if (autoClose) {