Add timeout to awaitGc (#10081)

open-telemetry · Dec 15, 2023 · fda1ee1 · fda1ee1
1 parent 586eea7
commit fda1ee1
Show file tree

Hide file tree

Showing 9 changed files with 49 additions and 29 deletions.
diff --git a/...nal-class-loader/javaagent-integration-tests/src/test/groovy/ResourceInjectionTest.groovy b/...nal-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/...ing/src/main/java/io/opentelemetry/instrumentation/micrometer/v1_5/AbstractGaugeTest.java b/...ing/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 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 @@ void gaugesWithSameNameAndDifferentTags() {
   }
 
   @Test
-  void testWeakRefGauge() throws InterruptedException {
+  void testWeakRefGauge() throws InterruptedException, TimeoutException {
     // given
     AtomicLong num = new AtomicLong(42);
     Gauge.builder("testWeakRefGauge", num, AtomicLong::get)
@@ -175,22 +178,12 @@ void testWeakRefGauge() throws InterruptedException {
     // when
     WeakReference<AtomicLong> numWeakRef = new WeakReference<>(num);
     num = null;
-    awaitGc(numWeakRef);
+    awaitGc(numWeakRef, Duration.ofSeconds(10));
     testing().clearData();
 
     // then
     testing()
         .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
@@ -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/...t-tooling/src/test/java/io/opentelemetry/javaagent/tooling/util/ClassLoaderValueTest.java b/...t-tooling/src/test/java/io/opentelemetry/javaagent/tooling/util/ClassLoaderValueTest.java
@@ -9,6 +9,8 @@
 
 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 @@ void testClassLoader(ClassLoader classLoader) {
   }
 
   @Test
-  void testGc() throws InterruptedException {
+  void testGc() throws InterruptedException, TimeoutException {
     ClassLoader testClassLoader = new ClassLoader() {};
     ClassLoaderValue<Value> classLoaderValue = new ClassLoaderValue<>();
     Value value = new Value();
@@ -40,8 +42,8 @@ void testGc() throws InterruptedException {
     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
@@ -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
@@ -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/.../src/test/java/io/opentelemetry/javaagent/tooling/muzzle/MuzzleWeakReferenceTestUtil.java b/.../src/test/java/io/opentelemetry/javaagent/tooling/muzzle/MuzzleWeakReferenceTestUtil.java
@@ -9,15 +9,18 @@
 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 {
 
   // 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<ClassLoader> 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<ClassLoader> clRef = new WeakReference<>(loader);
     ReferenceCollector collector = new ReferenceCollector(className -> false);
@@ -27,7 +30,7 @@ public static boolean classLoaderRefIsGarbageCollected() throws InterruptedExcep
             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/...ing-common/integration-tests/src/test/groovy/context/FieldBackedImplementationTest.groovy b/...ing-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<KeyClass> 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
@@ -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<Object> 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() {}