From c8da5812a9b33a0c2b02768d0122998f87d35918 Mon Sep 17 00:00:00 2001 From: Nick Tindall Date: Wed, 19 Jun 2024 10:39:23 +1000 Subject: [PATCH] [TEST] Add test for LeakTracker (#109777) Related to #101300 --- .../action/ActionListenerTests.java | 2 +- .../transport/LeakTrackerTests.java | 137 ++++++++++++++++++ .../org/elasticsearch/test/ESTestCase.java | 15 +- 3 files changed, 143 insertions(+), 11 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/transport/LeakTrackerTests.java diff --git a/server/src/test/java/org/elasticsearch/action/ActionListenerTests.java b/server/src/test/java/org/elasticsearch/action/ActionListenerTests.java index 1e18b71c99090..0543bce08a4f0 100644 --- a/server/src/test/java/org/elasticsearch/action/ActionListenerTests.java +++ b/server/src/test/java/org/elasticsearch/action/ActionListenerTests.java @@ -380,7 +380,7 @@ public void testAssertAtLeastOnceWillLogAssertionErrorWhenNotResolved() throws E listenerRef = null; assertBusy(() -> { System.gc(); - assertLeakDetected("LEAK: resource was not cleaned up before it was garbage-collected\\.(.*|\\s)*"); + assertLeakDetected(); }); } diff --git a/server/src/test/java/org/elasticsearch/transport/LeakTrackerTests.java b/server/src/test/java/org/elasticsearch/transport/LeakTrackerTests.java new file mode 100644 index 0000000000000..f13858351b7ee --- /dev/null +++ b/server/src/test/java/org/elasticsearch/transport/LeakTrackerTests.java @@ -0,0 +1,137 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.transport; + +import com.carrotsearch.randomizedtesting.annotations.Name; +import com.carrotsearch.randomizedtesting.annotations.ParametersFactory; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.elasticsearch.core.AbstractRefCounted; +import org.elasticsearch.core.Assertions; +import org.elasticsearch.core.RefCounted; +import org.elasticsearch.core.Releasables; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.test.ReachabilityChecker; +import org.junit.Before; + +import java.io.Closeable; +import java.io.IOException; +import java.util.stream.Stream; + +public class LeakTrackerTests extends ESTestCase { + + private static final Logger logger = LogManager.getLogger(); + + private final TrackedObjectLifecycle trackedObjectLifecycle; + private ReachabilityChecker reachabilityChecker; + + @Before + public void createReachabilityTracker() { + reachabilityChecker = new ReachabilityChecker(); + } + + @Before + public void onlyRunWhenAssertionsAreEnabled() { + assumeTrue("Many of these tests don't make sense when assertions are disabled", Assertions.ENABLED); + } + + public LeakTrackerTests(@Name("trackingMethod") TrackedObjectLifecycle trackedObjectLifecycle) { + this.trackedObjectLifecycle = trackedObjectLifecycle; + } + + @ParametersFactory(shuffle = false) + public static Iterable parameters() { + return Stream.of( + new PojoTrackedObjectLifecycle(), + new ReleasableTrackedObjectLifecycle(), + new ReferenceCountedTrackedObjectLifecycle() + ).map(i -> new Object[] { i }).toList(); + } + + @SuppressWarnings("resource") + public void testWillLogErrorWhenTrackedObjectIsNotClosed() throws Exception { + // Let it go out of scope without closing + trackedObjectLifecycle.createAndTrack(reachabilityChecker); + reachabilityChecker.ensureUnreachable(); + assertBusy(ESTestCase::assertLeakDetected); + } + + public void testWillNotLogErrorWhenTrackedObjectIsClosed() throws IOException { + // Close before letting it go out of scope + trackedObjectLifecycle.createAndTrack(reachabilityChecker).close(); + reachabilityChecker.ensureUnreachable(); + } + + /** + * Encapsulates the lifecycle for a particular type of tracked object + */ + public interface TrackedObjectLifecycle { + + /** + * Create the tracked object, implementations must + * - track it with the {@link LeakTracker} + * - register it with the passed reachability checker + * @param reachabilityChecker The reachability checker + * @return A {@link Closeable} that retains a reference to the tracked object, and when closed will do the appropriate cleanup + */ + Closeable createAndTrack(ReachabilityChecker reachabilityChecker); + } + + private static class PojoTrackedObjectLifecycle implements TrackedObjectLifecycle { + + @Override + public Closeable createAndTrack(ReachabilityChecker reachabilityChecker) { + final Object object = reachabilityChecker.register(new Object()); + final LeakTracker.Leak leak = LeakTracker.INSTANCE.track(object); + return () -> { + logger.info("This log line retains a reference to {}", object); + leak.close(); + }; + } + + @Override + public String toString() { + return "LeakTracker.INSTANCE.track(Object)"; + } + } + + private static class ReferenceCountedTrackedObjectLifecycle implements TrackedObjectLifecycle { + + @Override + public Closeable createAndTrack(ReachabilityChecker reachabilityChecker) { + RefCounted refCounted = LeakTracker.wrap(reachabilityChecker.register((RefCounted) AbstractRefCounted.of(() -> {}))); + refCounted.incRef(); + refCounted.tryIncRef(); + return () -> { + refCounted.decRef(); // tryIncRef + refCounted.decRef(); // incRef + refCounted.decRef(); // implicit + }; + } + + @Override + public String toString() { + return "LeakTracker.wrap(RefCounted)"; + } + } + + private static class ReleasableTrackedObjectLifecycle implements TrackedObjectLifecycle { + + @Override + public Closeable createAndTrack(ReachabilityChecker reachabilityChecker) { + return LeakTracker.wrap(reachabilityChecker.register(Releasables.assertOnce(() -> {}))); + } + + @Override + public String toString() { + return "LeakTracker.wrap(Releasable)"; + } + } +} diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java index 8ae5cdd8b9217..46d5e9441b83d 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java @@ -186,7 +186,6 @@ import java.util.function.IntFunction; import java.util.function.Predicate; import java.util.function.Supplier; -import java.util.regex.Pattern; import java.util.stream.Collectors; import java.util.stream.DoubleStream; import java.util.stream.IntStream; @@ -790,17 +789,13 @@ protected static void checkStaticState() throws Exception { } /** - * Assert that a leak was detected, also remove the leak from the list of detected leaks - * so the test won't fail for that specific leak. - * - * @param expectedPattern A pattern that matches the detected leak's exception + * Assert that at least one leak was detected, also clear the list of detected leaks + * so the test won't fail for leaks detected up until this point. */ - protected static void assertLeakDetected(String expectedPattern) { + protected static void assertLeakDetected() { synchronized (loggedLeaks) { - assertTrue( - "No leak detected matching the pattern: " + expectedPattern, - loggedLeaks.removeIf(leakText -> Pattern.matches(expectedPattern, leakText)) - ); + assertFalse("No leaks have been detected", loggedLeaks.isEmpty()); + loggedLeaks.clear(); } }