Validate that tests don't leaks tracers, scopes and pooled objects

apm-agent-core/src/main/java/co/elastic/apm/agent/bci/ElasticApmAgent.java

@@ -205,7 +205,7 @@ private static synchronized void initInstrumentation(final ElasticApmTracer trac
         if (!tracer.getConfig(CoreConfiguration.class).isEnabled()) {
             return;
         }
-        GlobalTracer.set(tracer);
+        GlobalTracer.init(tracer);
         for (ElasticApmInstrumentation apmInstrumentation : instrumentations) {
             pluginClassLoaderByAdviceClass.put(
                 apmInstrumentation.getAdviceClass().getName(),
@@ -520,6 +520,8 @@ public static synchronized void reset() {
         if (instrumentation == null) {
             return;
         }
+        GlobalTracer.get().stop();
+        GlobalTracer.setNoop();
         Exception exception = null;
         if (resettableClassFileTransformer != null) {
             try {

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/GlobalTracer.java

@@ -31,16 +31,12 @@
 import co.elastic.apm.agent.impl.transaction.Span;
 import co.elastic.apm.agent.impl.transaction.TextHeaderGetter;
 import co.elastic.apm.agent.impl.transaction.Transaction;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 import javax.annotation.Nullable;
 import java.util.Objects;
 
 public class GlobalTracer implements Tracer {
 
-    private static final Logger logger = LoggerFactory.getLogger(GlobalTracer.class);
-
     private static final GlobalTracer INSTANCE = new GlobalTracer();
     private volatile Tracer tracer = NoopTracer.INSTANCE;
 
@@ -64,20 +60,25 @@ public static ElasticApmTracer requireTracerImpl() {
         return Objects.requireNonNull(getTracerImpl(), "Registered tracer is not an instance of ElasticApmTracer");
     }
 
-    public static void setNoop() {
-        set(NoopTracer.INSTANCE);
-    }
-
-    public static void set(Tracer tracer) {
+    public static synchronized void setNoop() {
         TracerState currentTracerState = INSTANCE.tracer.getState();
         if (currentTracerState != TracerState.UNINITIALIZED && currentTracerState != TracerState.STOPPED) {
-            logger.warn("Overriding running tracer");
-            // TODO throw exception, requires changes in tests
-            // throw new IllegalStateException("Can't override tracer as current tracer is already running");
+             throw new IllegalStateException("Can't override tracer as current tracer is already running");
+        }
+        INSTANCE.tracer = NoopTracer.INSTANCE;
+    }
+
+    public static synchronized void init(Tracer tracer) {
+        if (!isNoop()) {
+             throw new IllegalStateException("Tracer is already initialized");
         }
         INSTANCE.tracer = tracer;
     }
 
+    public static boolean isNoop() {
+        return INSTANCE.tracer == NoopTracer.INSTANCE;
+    }
+
     @Nullable
     public Transaction startRootTransaction(@Nullable ClassLoader initiatingClassLoader) {
         return tracer.startRootTransaction(initiatingClassLoader);

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/context/Destination.java

@@ -143,7 +143,7 @@ public boolean hasContent() {
     @Override
     public void resetState() {
         address.setLength(0);
-        port = -1;
+        port = 0;
         service.resetState();
     }
 

apm-agent-core/src/test/java/co/elastic/apm/agent/AbstractInstrumentationTest.java

@@ -26,19 +26,17 @@
 
 import co.elastic.apm.agent.bci.ElasticApmAgent;
 import co.elastic.apm.agent.configuration.SpyConfiguration;
-import co.elastic.apm.agent.impl.Tracer;
 import co.elastic.apm.agent.impl.ElasticApmTracer;
+import co.elastic.apm.agent.impl.Tracer;
 import co.elastic.apm.agent.impl.TracerInternalApiUtils;
 import co.elastic.apm.agent.objectpool.TestObjectPoolFactory;
 import net.bytebuddy.agent.ByteBuddyAgent;
 import org.junit.After;
 import org.junit.AfterClass;
-import org.junit.Before;
 import org.junit.BeforeClass;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeAll;
-import org.junit.jupiter.api.BeforeEach;
 import org.stagemonitor.configuration.ConfigurationRegistry;
 
 import static org.assertj.core.api.Assertions.assertThat;
@@ -48,11 +46,12 @@ public abstract class AbstractInstrumentationTest {
     protected static MockReporter reporter;
     protected static ConfigurationRegistry config;
     protected static TestObjectPoolFactory objectPoolFactory;
+    private boolean validateRecycling = true;
 
     @BeforeAll
     @BeforeClass
     public static synchronized void beforeAll() {
-        MockTracer.MockInstrumentationSetup mockInstrumentationSetup = MockTracer.getOrCreateInstrumentationTracer();
+        MockTracer.MockInstrumentationSetup mockInstrumentationSetup = MockTracer.createMockInstrumentationSetup();
         tracer = mockInstrumentationSetup.getTracer();
         config = mockInstrumentationSetup.getConfig();
         objectPoolFactory = mockInstrumentationSetup.getObjectPoolFactory();
@@ -67,15 +66,8 @@ public static synchronized void afterAll() {
         ElasticApmAgent.reset();
     }
 
-    public static synchronized void staticReset() {
-        SpyConfiguration.reset(config);
-        reporter.reset();
-
-        // resume tracer in case it has been paused
-        // otherwise the 1st test that pauses tracer will have side effects on others
-        if (!tracer.isRunning()) {
-            TracerInternalApiUtils.resumeTracer(tracer);
-        }
+    protected void disableRecyclingValidation() {
+        validateRecycling = false;
     }
 
     public static Tracer getTracer() {
@@ -90,15 +82,25 @@ public static ConfigurationRegistry getConfig() {
         return config;
     }
 
-    @Before
-    @BeforeEach
-    public final void reset() {
-        staticReset();
-    }
-
     @After
     @AfterEach
     public final void cleanUp() {
+        SpyConfiguration.reset(config);
+        try {
+            if (validateRecycling) {
+                reporter.assertRecycledAfterDecrementingReferences();
+                objectPoolFactory.checkAllPooledObjectsHaveBeenRecycled();
+            }
+        } finally {
+            // one faulty test should not affect others
+            reporter.resetWithoutRecycling();
+            objectPoolFactory.reset();
+            // resume tracer in case it has been paused
+            // otherwise the 1st test that pauses tracer will have side effects on others
+            if (!tracer.isRunning()) {
+                TracerInternalApiUtils.resumeTracer(tracer);
+            }
+        }
         tracer.resetServiceNameOverrides();
 
         assertThat(tracer.getActive())

apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java

@@ -42,7 +42,6 @@
 import com.networknt.schema.JsonSchema;
 import com.networknt.schema.JsonSchemaFactory;
 import com.networknt.schema.ValidationMessage;
-import org.awaitility.core.ConditionFactory;
 import org.awaitility.core.ThrowingRunnable;
 
 import java.io.IOException;
@@ -56,10 +55,10 @@
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
+import java.util.concurrent.locks.LockSupport;
 import java.util.stream.Collectors;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.awaitility.Awaitility.await;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
@@ -197,8 +196,7 @@ public synchronized Transaction getFirstTransaction() {
     }
 
     public Transaction getFirstTransaction(long timeoutMs) {
-        awaitTimeout(timeoutMs)
-            .untilAsserted(() -> assertThat(getTransactions()).isNotEmpty());
+        awaitUntilAsserted(timeoutMs, () -> assertThat(getTransactions()).isNotEmpty());
         return getFirstTransaction();
     }
 
@@ -209,24 +207,11 @@ public void assertNoTransaction() {
     }
 
     public void assertNoTransaction(long timeoutMs) {
-        awaitTimeout(timeoutMs)
-            .untilAsserted(this::assertNoTransaction);
-    }
-
-    public void awaitUntilAsserted(long timeoutMs, ThrowingRunnable assertion){
-        awaitTimeout(timeoutMs)
-            .untilAsserted(assertion);
-    }
-
-    private static ConditionFactory awaitTimeout(long timeoutMs) {
-        return await()
-            .pollInterval(1, TimeUnit.MILLISECONDS)
-            .timeout(timeoutMs, TimeUnit.MILLISECONDS);
+        awaitUntilAsserted(timeoutMs, this::assertNoTransaction);
     }
 
     public Span getFirstSpan(long timeoutMs) {
-        awaitTimeout(timeoutMs)
-            .untilAsserted(() -> assertThat(getSpans()).isNotEmpty());
+        awaitUntilAsserted(timeoutMs, () -> assertThat(getSpans()).isNotEmpty());
         return getFirstSpan();
     }
 
@@ -237,30 +222,17 @@ public void assertNoSpan() {
     }
 
     public void assertNoSpan(long timeoutMs) {
-        awaitTimeout(timeoutMs)
-            .untilAsserted(() -> assertThat(getSpans()).isEmpty());
+        awaitUntilAsserted(timeoutMs, () -> assertThat(getSpans()).isEmpty());
 
         assertNoSpan();
     }
 
     public void awaitTransactionCount(int count) {
-        awaitTimeout(1000)
-            .untilAsserted(() -> assertThat(getNumReportedTransactions()).isEqualTo(count));
-    }
-
-    public void awaitTransactionReported() {
-        awaitTimeout(1000)
-            .untilAsserted(() -> assertThat(getNumReportedTransactions()).isGreaterThan(0));
+        awaitUntilAsserted(() -> assertThat(getNumReportedTransactions()).isEqualTo(count));
     }
 
     public void awaitSpanCount(int count) {
-        awaitTimeout(1000)
-            .untilAsserted(() -> assertThat(getNumReportedSpans()).isEqualTo(count));
-    }
-
-    public void awaitSpanReported() {
-        awaitTimeout(1000)
-            .untilAsserted(() -> assertThat(getNumReportedSpans()).isGreaterThan(0));
+        awaitUntilAsserted(() -> assertThat(getNumReportedSpans()).isEqualTo(count));
     }
 
     @Override
@@ -349,6 +321,11 @@ public synchronized void close() {
     }
 
     public synchronized void reset() {
+        assertRecycledAfterDecrementingReferences();
+        resetWithoutRecycling();
+    }
+
+    public synchronized void resetWithoutRecycling() {
         transactions.clear();
         spans.clear();
         errors.clear();
@@ -383,33 +360,62 @@ public synchronized void assertRecycledAfterDecrementingReferences() {
         transactionsToFlush.forEach(Transaction::decrementReferences);
         spansToFlush.forEach(Span::decrementReferences);
 
-        // transactions might be active after they have already been reported
-        // after a short amount of time, all transactions and spans should have been recycled
-        await()
-            .timeout(1, TimeUnit.SECONDS)
-            .untilAsserted(() -> transactions.forEach(t -> {
-                assertThat(hasEmptyTraceContext(t))
-                    .describedAs("should have empty trace context : %s", t)
-                    .isTrue();
-                assertThat(t.isReferenced())
-                    .describedAs("should not have any reference left, but has %d : %s", t.getReferenceCount(), t)
+        awaitUntilAsserted(() -> {
+            spans.forEach(s -> {
+                assertThat(s.isReferenced())
+                    .describedAs("should not have any reference left, but has %d : %s", s.getReferenceCount(), s)
                     .isFalse();
-            }));
-        await()
-            .timeout(1, TimeUnit.SECONDS)
-            .untilAsserted(() -> spans.forEach(s -> {
                 assertThat(hasEmptyTraceContext(s))
                     .describedAs("should have empty trace context : %s", s)
                     .isTrue();
-                assertThat(s.isReferenced())
-                    .describedAs("should not have any reference left, but has %d : %s", s.getReferenceCount(), s)
+            });
+            transactions.forEach(t -> {
+                assertThat(t.isReferenced())
+                    .describedAs("should not have any reference left, but has %d : %s", t.getReferenceCount(), t)
                     .isFalse();
-            }));
+                assertThat(hasEmptyTraceContext(t))
+                    .describedAs("should have empty trace context : %s", t)
+                    .isTrue();
+            });
+        });
 
         // errors are recycled directly because they have no reference counter
         errors.forEach(ErrorCapture::recycle);
     }
 
+    /**
+     * Uses a timeout of 1s
+     *
+     * @see #awaitUntilAsserted(long, ThrowingRunnable)
+     */
+    public void awaitUntilAsserted(ThrowingRunnable runnable) {
+        awaitUntilAsserted(1000, runnable);
+    }
+
+    /**
+     * This is deliberately not using {@link org.awaitility.Awaitility} as it uses an {@link java.util.concurrent.Executor} for polling.
+     * This is an issue when testing instrumentations that instrument {@link java.util.concurrent.Executor}.
+     *
+     * @param timeoutMs the timeout of the condition
+     * @param runnable  a runnable that trows an exception if the condition is not met
+     */
+    public void awaitUntilAsserted(long timeoutMs, ThrowingRunnable runnable) {
+        Throwable thrown = null;
+        for (int i = 0; i < timeoutMs; i += 5) {
+            try {
+                runnable.run();
+                thrown = null;
+            } catch (Throwable e) {
+                thrown = e;
+                LockSupport.parkNanos(TimeUnit.MILLISECONDS.toNanos(5));
+            }
+        }
+        if (thrown != null) {
+            throw new RuntimeException(String.format("Condition not fulfilled within %d ms", timeoutMs), thrown);
+        }
+    }
+
+
     private static boolean hasEmptyTraceContext(AbstractSpan<?> item) {
         return item.getTraceContext().getId().isEmpty();
     }