From fda1ee124a9167beb7b86bf63bff869d75eac78c Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Fri, 15 Dec 2023 21:45:35 +0200 Subject: [PATCH] Add timeout to awaitGc (#10081) --- .../test/groovy/ResourceInjectionTest.groovy | 3 ++- .../micrometer/v1_5/AbstractGaugeTest.java | 17 +++++------------ .../javaagent/test/HelperInjectionTest.groovy | 7 ++++--- .../tooling/util/ClassLoaderValueTest.java | 8 +++++--- .../classloading/ClassLoadingTest.groovy | 3 ++- .../opentelemetry/javaagent/util/GcUtils.java | 13 +++++++++++-- .../muzzle/MuzzleWeakReferenceTestUtil.java | 7 +++++-- .../FieldBackedImplementationTest.groovy | 3 ++- .../instrumentation/test/utils/GcUtils.java | 17 +++++++++++++---- 9 files changed, 49 insertions(+), 29 deletions(-) diff --git a/instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/test/groovy/ResourceInjectionTest.groovy b/instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/test/groovy/ResourceInjectionTest.groovy index df88737e99..b5b3d799c4 100644 --- a/instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/test/groovy/ResourceInjectionTest.groovy +++ b/instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/test/groovy/ResourceInjectionTest.groovy @@ -7,6 +7,7 @@ import io.opentelemetry.instrumentation.test.AgentInstrumentationSpecification import org.apache.commons.lang3.SystemUtils import java.lang.ref.WeakReference +import java.time.Duration import java.util.concurrent.atomic.AtomicReference import static io.opentelemetry.instrumentation.test.utils.GcUtils.awaitGc @@ -44,7 +45,7 @@ class ResourceInjectionTest extends AgentInstrumentationSpecification { def ref = new WeakReference(emptyLoader.get()) emptyLoader.set(null) - awaitGc(ref) + awaitGc(ref, Duration.ofSeconds(10)) then: "HelperInjector doesn't prevent it from being collected" null == ref.get() diff --git a/instrumentation/micrometer/micrometer-1.5/testing/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/AbstractGaugeTest.java b/instrumentation/micrometer/micrometer-1.5/testing/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/AbstractGaugeTest.java index 752fbb3ab3..550537c33b 100644 --- a/instrumentation/micrometer/micrometer-1.5/testing/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/AbstractGaugeTest.java +++ b/instrumentation/micrometer/micrometer-1.5/testing/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/AbstractGaugeTest.java @@ -6,6 +6,7 @@ package io.opentelemetry.instrumentation.micrometer.v1_5; import static io.opentelemetry.instrumentation.micrometer.v1_5.AbstractCounterTest.INSTRUMENTATION_NAME; +import static io.opentelemetry.instrumentation.test.utils.GcUtils.awaitGc; import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat; import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.attributeEntry; @@ -15,6 +16,8 @@ import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension; import java.lang.ref.WeakReference; import java.net.URL; import java.net.URLClassLoader; +import java.time.Duration; +import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicLong; import org.assertj.core.api.AbstractIterableAssert; import org.junit.jupiter.api.Test; @@ -153,7 +156,7 @@ public abstract class AbstractGaugeTest { } @Test - void testWeakRefGauge() throws InterruptedException { + void testWeakRefGauge() throws InterruptedException, TimeoutException { // given AtomicLong num = new AtomicLong(42); Gauge.builder("testWeakRefGauge", num, AtomicLong::get) @@ -175,7 +178,7 @@ public abstract class AbstractGaugeTest { // when WeakReference numWeakRef = new WeakReference<>(num); num = null; - awaitGc(numWeakRef); + awaitGc(numWeakRef, Duration.ofSeconds(10)); testing().clearData(); // then @@ -183,14 +186,4 @@ public abstract class AbstractGaugeTest { .waitAndAssertMetrics( INSTRUMENTATION_NAME, "testWeakRefGauge", AbstractIterableAssert::isEmpty); } - - private static void awaitGc(WeakReference ref) throws InterruptedException { - while (ref.get() != null) { - if (Thread.interrupted()) { - throw new InterruptedException(); - } - System.gc(); - System.runFinalization(); - } - } } diff --git a/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/test/HelperInjectionTest.groovy b/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/test/HelperInjectionTest.groovy index 1bada09485..7f2882806c 100644 --- a/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/test/HelperInjectionTest.groovy +++ b/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/test/HelperInjectionTest.groovy @@ -17,6 +17,7 @@ import net.bytebuddy.dynamic.loading.ClassInjector import spock.lang.Specification import java.lang.ref.WeakReference +import java.time.Duration import java.util.concurrent.atomic.AtomicReference import static io.opentelemetry.instrumentation.test.utils.ClasspathUtils.isClassLoaded @@ -53,7 +54,7 @@ class HelperInjectionTest extends Specification { def ref = new WeakReference(emptyLoader.get()) emptyLoader.set(null) - awaitGc(ref) + awaitGc(ref, Duration.ofSeconds(10)) then: "HelperInjector doesn't prevent it from being collected" null == ref.get() @@ -100,7 +101,7 @@ class HelperInjectionTest extends Specification { def injectorRef = new WeakReference(injector.get()) injector.set(null) - awaitGc(injectorRef) + awaitGc(injectorRef, Duration.ofSeconds(10)) then: null == injectorRef.get() @@ -109,7 +110,7 @@ class HelperInjectionTest extends Specification { def loaderRef = new WeakReference(emptyLoader.get()) emptyLoader.set(null) - awaitGc(loaderRef) + awaitGc(loaderRef, Duration.ofSeconds(10)) then: null == loaderRef.get() diff --git a/javaagent-tooling/src/test/java/io/opentelemetry/javaagent/tooling/util/ClassLoaderValueTest.java b/javaagent-tooling/src/test/java/io/opentelemetry/javaagent/tooling/util/ClassLoaderValueTest.java index 799248f8db..b4a3cac845 100644 --- a/javaagent-tooling/src/test/java/io/opentelemetry/javaagent/tooling/util/ClassLoaderValueTest.java +++ b/javaagent-tooling/src/test/java/io/opentelemetry/javaagent/tooling/util/ClassLoaderValueTest.java @@ -9,6 +9,8 @@ import static org.assertj.core.api.Assertions.assertThat; import io.opentelemetry.instrumentation.test.utils.GcUtils; import java.lang.ref.WeakReference; +import java.time.Duration; +import java.util.concurrent.TimeoutException; import org.junit.jupiter.api.Test; class ClassLoaderValueTest { @@ -26,7 +28,7 @@ class ClassLoaderValueTest { } @Test - void testGc() throws InterruptedException { + void testGc() throws InterruptedException, TimeoutException { ClassLoader testClassLoader = new ClassLoader() {}; ClassLoaderValue classLoaderValue = new ClassLoaderValue<>(); Value value = new Value(); @@ -40,8 +42,8 @@ class ClassLoaderValueTest { value = null; testClassLoader = null; - GcUtils.awaitGc(classLoaderWeakReference); - GcUtils.awaitGc(valueWeakReference); + GcUtils.awaitGc(classLoaderWeakReference, Duration.ofSeconds(10)); + GcUtils.awaitGc(valueWeakReference, Duration.ofSeconds(10)); assertThat(classLoaderWeakReference.get()).isNull(); assertThat(valueWeakReference.get()).isNull(); diff --git a/javaagent/src/test/groovy/io/opentelemetry/javaagent/classloading/ClassLoadingTest.groovy b/javaagent/src/test/groovy/io/opentelemetry/javaagent/classloading/ClassLoadingTest.groovy index 5373c2dfc7..b6a6042da8 100644 --- a/javaagent/src/test/groovy/io/opentelemetry/javaagent/classloading/ClassLoadingTest.groovy +++ b/javaagent/src/test/groovy/io/opentelemetry/javaagent/classloading/ClassLoadingTest.groovy @@ -11,6 +11,7 @@ import io.opentelemetry.javaagent.util.GcUtils import spock.lang.Specification import java.lang.ref.WeakReference +import java.time.Duration import static io.opentelemetry.javaagent.IntegrationTestUtils.createJarWithClasses @@ -43,7 +44,7 @@ class ClassLoadingTest extends Specification { loader.loadClass(ClassToInstrument.getName()) loader = null - GcUtils.awaitGc(ref) + GcUtils.awaitGc(ref, Duration.ofSeconds(10)) then: null == ref.get() diff --git a/javaagent/src/test/java/io/opentelemetry/javaagent/util/GcUtils.java b/javaagent/src/test/java/io/opentelemetry/javaagent/util/GcUtils.java index c3dafe56bc..9ee3dc059f 100644 --- a/javaagent/src/test/java/io/opentelemetry/javaagent/util/GcUtils.java +++ b/javaagent/src/test/java/io/opentelemetry/javaagent/util/GcUtils.java @@ -6,16 +6,25 @@ package io.opentelemetry.javaagent.util; import java.lang.ref.WeakReference; +import java.time.Duration; +import java.time.temporal.ChronoUnit; +import java.util.concurrent.TimeoutException; public final class GcUtils { - public static void awaitGc(WeakReference ref) throws InterruptedException { - while (ref.get() != null) { + public static void awaitGc(WeakReference ref, Duration timeout) + throws InterruptedException, TimeoutException { + long start = System.currentTimeMillis(); + while (ref.get() != null + && !timeout.minus(System.currentTimeMillis() - start, ChronoUnit.MILLIS).isNegative()) { if (Thread.interrupted()) { throw new InterruptedException(); } System.gc(); System.runFinalization(); } + if (ref.get() != null) { + throw new TimeoutException("reference was not cleared in time"); + } } private GcUtils() {} diff --git a/muzzle/src/test/java/io/opentelemetry/javaagent/tooling/muzzle/MuzzleWeakReferenceTestUtil.java b/muzzle/src/test/java/io/opentelemetry/javaagent/tooling/muzzle/MuzzleWeakReferenceTestUtil.java index d23ad66a6e..40695dccd6 100644 --- a/muzzle/src/test/java/io/opentelemetry/javaagent/tooling/muzzle/MuzzleWeakReferenceTestUtil.java +++ b/muzzle/src/test/java/io/opentelemetry/javaagent/tooling/muzzle/MuzzleWeakReferenceTestUtil.java @@ -9,7 +9,9 @@ import io.opentelemetry.instrumentation.test.utils.GcUtils; import java.lang.ref.WeakReference; import java.net.URL; import java.net.URLClassLoader; +import java.time.Duration; import java.util.Collections; +import java.util.concurrent.TimeoutException; import muzzle.TestClasses.MethodBodyAdvice; public class MuzzleWeakReferenceTestUtil { @@ -17,7 +19,8 @@ public class MuzzleWeakReferenceTestUtil { // Spock holds strong references to all local variables. For weak reference testing we must create // our strong references away from Spock in this java class. // Even returning a WeakReference is enough for spock to create a strong ref. - public static boolean classLoaderRefIsGarbageCollected() throws InterruptedException { + public static boolean classLoaderRefIsGarbageCollected() + throws InterruptedException, TimeoutException { ClassLoader loader = new URLClassLoader(new URL[0], null); WeakReference clRef = new WeakReference<>(loader); ReferenceCollector collector = new ReferenceCollector(className -> false); @@ -27,7 +30,7 @@ public class MuzzleWeakReferenceTestUtil { Collections.emptyList(), collector.getReferences(), className -> false); refMatcher.getMismatchedReferenceSources(loader); loader = null; - GcUtils.awaitGc(clRef); + GcUtils.awaitGc(clRef, Duration.ofSeconds(10)); return clRef.get() == null; } diff --git a/testing-common/integration-tests/src/test/groovy/context/FieldBackedImplementationTest.groovy b/testing-common/integration-tests/src/test/groovy/context/FieldBackedImplementationTest.groovy index d45bf83238..b74ed226b4 100644 --- a/testing-common/integration-tests/src/test/groovy/context/FieldBackedImplementationTest.groovy +++ b/testing-common/integration-tests/src/test/groovy/context/FieldBackedImplementationTest.groovy @@ -22,6 +22,7 @@ import java.lang.ref.WeakReference import java.lang.reflect.Field import java.lang.reflect.Method import java.lang.reflect.Modifier +import java.time.Duration import java.util.concurrent.atomic.AtomicReference // this test is run using @@ -182,7 +183,7 @@ class FieldBackedImplementationTest extends AgentInstrumentationSpecification { int count = keyValue.get().incrementContextCount() WeakReference instanceRef = new WeakReference(keyValue.get()) keyValue.set(null) - GcUtils.awaitGc(instanceRef) + GcUtils.awaitGc(instanceRef, Duration.ofSeconds(10)) then: instanceRef.get() == null diff --git a/testing-common/src/main/java/io/opentelemetry/instrumentation/test/utils/GcUtils.java b/testing-common/src/main/java/io/opentelemetry/instrumentation/test/utils/GcUtils.java index d8f6866c17..42bba2b0b8 100644 --- a/testing-common/src/main/java/io/opentelemetry/instrumentation/test/utils/GcUtils.java +++ b/testing-common/src/main/java/io/opentelemetry/instrumentation/test/utils/GcUtils.java @@ -6,24 +6,33 @@ package io.opentelemetry.instrumentation.test.utils; import java.lang.ref.WeakReference; +import java.time.Duration; +import java.time.temporal.ChronoUnit; +import java.util.concurrent.TimeoutException; public final class GcUtils { - public static void awaitGc() throws InterruptedException { + public static void awaitGc(Duration timeout) throws InterruptedException, TimeoutException { Object obj = new Object(); WeakReference ref = new WeakReference<>(obj); obj = null; - awaitGc(ref); + awaitGc(ref, timeout); } - public static void awaitGc(WeakReference ref) throws InterruptedException { - while (ref.get() != null) { + public static void awaitGc(WeakReference ref, Duration timeout) + throws InterruptedException, TimeoutException { + long start = System.currentTimeMillis(); + while (ref.get() != null + && !timeout.minus(System.currentTimeMillis() - start, ChronoUnit.MILLIS).isNegative()) { if (Thread.interrupted()) { throw new InterruptedException(); } System.gc(); System.runFinalization(); } + if (ref.get() != null) { + throw new TimeoutException("reference was not cleared in time"); + } } private GcUtils() {}