Make MockLogAppender itself Releasable (#108526)

Existing uses of MockLogAppender first construct an appender, then call
capturing on the instance in a try-with-resources block. This commit
adds a new method, capture, which creates an appender and sets up the
capture the the same time. The intent is that this will replace the
existing capturing calls, but there are too many to change in one PR.

modules/transport-netty4/src/internalClusterTest/java/org/elasticsearch/transport/netty4/ESLoggingHandlerIT.java

@@ -10,7 +10,6 @@ package org.elasticsearch.transport.netty4;
 
 import org.apache.logging.log4j.Level;
 import org.elasticsearch.ESNetty4IntegTestCase;
-import org.elasticsearch.core.Releasable;
 import org.elasticsearch.core.TimeValue;
 import org.elasticsearch.test.ESIntegTestCase;
 import org.elasticsearch.test.MockLogAppender;
@@ -24,16 +23,14 @@ import java.io.IOException;
 public class ESLoggingHandlerIT extends ESNetty4IntegTestCase {
 
     private MockLogAppender appender;
-    private Releasable appenderRelease;
 
     public void setUp() throws Exception {
         super.setUp();
-        appender = new MockLogAppender();
+        appender = MockLogAppender.capture(ESLoggingHandler.class, TransportLogger.class, TcpTransport.class);
-        appenderRelease = appender.capturing(ESLoggingHandler.class, TransportLogger.class, TcpTransport.class);
     }
 
     public void tearDown() throws Exception {
-        appenderRelease.close();
+        appender.close();
         super.tearDown();
     }
 

qa/no-bootstrap-tests/src/test/java/org/elasticsearch/bootstrap/SpawnerNoBootstrapTests.java

@@ -206,16 +206,16 @@ public class SpawnerNoBootstrapTests extends LuceneTestCase {
 
         String stdoutLoggerName = "test_plugin-controller-stdout";
         String stderrLoggerName = "test_plugin-controller-stderr";
-        MockLogAppender appender = new MockLogAppender();
         Loggers.setLevel(LogManager.getLogger(stdoutLoggerName), Level.TRACE);
         Loggers.setLevel(LogManager.getLogger(stderrLoggerName), Level.TRACE);
         CountDownLatch messagesLoggedLatch = new CountDownLatch(2);
+
+        try (var appender = MockLogAppender.capture(stdoutLoggerName, stderrLoggerName)) {
             if (expectSpawn) {
                 appender.addExpectation(new ExpectedStreamMessage(stdoutLoggerName, "I am alive", messagesLoggedLatch));
                 appender.addExpectation(new ExpectedStreamMessage(stderrLoggerName, "I am an error", messagesLoggedLatch));
             }
 
-        try (var ignore = appender.capturing(stdoutLoggerName, stderrLoggerName)) {
             Spawner spawner = new Spawner();
             spawner.spawnNativeControllers(environment);
 

server/src/internalClusterTest/java/org/elasticsearch/cluster/allocation/ClusterRerouteIT.java

@@ -387,7 +387,7 @@ public class ClusterRerouteIT extends ESIntegTestCase {
             )
             .get();
 
-        MockLogAppender dryRunMockLog = new MockLogAppender();
+        try (var dryRunMockLog = MockLogAppender.capture(TransportClusterRerouteAction.class)) {
             dryRunMockLog.addExpectation(
                 new MockLogAppender.UnseenEventExpectation(
                     "no completed message logged on dry run",
@@ -397,7 +397,6 @@ public class ClusterRerouteIT extends ESIntegTestCase {
                 )
             );
 
-        try (var ignored = dryRunMockLog.capturing(TransportClusterRerouteAction.class)) {
             AllocationCommand dryRunAllocation = new AllocateEmptyPrimaryAllocationCommand(indexName, 0, nodeName1, true);
             ClusterRerouteResponse dryRunResponse = clusterAdmin().prepareReroute()
                 .setExplain(randomBoolean())
@@ -412,7 +411,7 @@ public class ClusterRerouteIT extends ESIntegTestCase {
             dryRunMockLog.assertAllExpectationsMatched();
         }
 
-        MockLogAppender allocateMockLog = new MockLogAppender();
+        try (var allocateMockLog = MockLogAppender.capture(TransportClusterRerouteAction.class)) {
             allocateMockLog.addExpectation(
                 new MockLogAppender.SeenEventExpectation(
                     "message for first allocate empty primary",
@@ -429,7 +428,6 @@ public class ClusterRerouteIT extends ESIntegTestCase {
                     "allocated an empty primary*" + nodeName2 + "*"
                 )
             );
-        try (var ignored = allocateMockLog.capturing(TransportClusterRerouteAction.class)) {
 
             AllocationCommand yesDecisionAllocation = new AllocateEmptyPrimaryAllocationCommand(indexName, 0, nodeName1, true);
             AllocationCommand noDecisionAllocation = new AllocateEmptyPrimaryAllocationCommand("noexist", 1, nodeName2, true);

test/framework/src/main/java/org/elasticsearch/test/MockLogAppender.java

@@ -31,13 +31,35 @@ import static org.hamcrest.Matchers.is;
 /**
  * Test appender that can be used to verify that certain events were logged correctly
  */
-public class MockLogAppender {
+public class MockLogAppender implements Releasable {
 
     private static final Map<String, List<MockLogAppender>> mockAppenders = new ConcurrentHashMap<>();
     private static final RealMockAppender parent = new RealMockAppender();
+    // TODO: this can become final once the ctor is made private
+    private List<String> loggers = List.of();
     private final List<WrappedLoggingExpectation> expectations;
     private volatile boolean isAlive = true;
 
+    @Override
+    public void close() {
+        isAlive = false;
+        for (String logger : loggers) {
+            mockAppenders.compute(logger, (k, v) -> {
+                assert v != null;
+                v.remove(this);
+                return v.isEmpty() ? null : v;
+            });
+        }
+        // check that all expectations have been evaluated before this is released
+        for (WrappedLoggingExpectation expectation : expectations) {
+            assertThat(
+                "Method assertMatched() not called on LoggingExpectation instance before release: " + expectation,
+                expectation.assertMatchedCalled,
+                is(true)
+            );
+        }
+    }
+
     private static class RealMockAppender extends AbstractAppender {
 
         RealMockAppender() {
@@ -71,6 +93,11 @@ public class MockLogAppender {
         expectations = new CopyOnWriteArrayList<>();
     }
 
+    private MockLogAppender(List<String> loggers) {
+        this();
+        this.loggers = loggers;
+    }
+
     /**
      * Initialize the mock log appender with the log4j system.
      */
@@ -267,58 +294,57 @@ public class MockLogAppender {
         }
     }
 
+    public Releasable capturing(Class<?>... classes) {
+        this.loggers = Arrays.stream(classes).map(Class::getCanonicalName).toList();
+        addToMockAppenders(this, loggers);
+        return this;
+    }
+
+    public Releasable capturing(String... names) {
+        this.loggers = Arrays.asList(names);
+        addToMockAppenders(this, loggers);
+        return this;
+    }
+
     /**
      * Adds the list of class loggers to this {@link MockLogAppender}.
      *
      * Stops and runs some checks on the {@link MockLogAppender} once the returned object is released.
      */
-    public Releasable capturing(Class<?>... classes) {
+    public static MockLogAppender capture(Class<?>... classes) {
-        return appendToLoggers(Arrays.stream(classes).map(Class::getCanonicalName).toList());
+        return create(Arrays.stream(classes).map(Class::getCanonicalName).toList());
     }
 
     /**
      * Same as above except takes string class names of each logger.
      */
-    public Releasable capturing(String... names) {
+    public static MockLogAppender capture(String... names) {
-        return appendToLoggers(Arrays.asList(names));
+        return create(Arrays.asList(names));
     }
 
-    private Releasable appendToLoggers(List<String> loggers) {
+    private static MockLogAppender create(List<String> loggers) {
+        MockLogAppender appender = new MockLogAppender(loggers);
+        addToMockAppenders(appender, loggers);
+        return appender;
+    }
+
+    private static void addToMockAppenders(MockLogAppender appender, List<String> loggers) {
         for (String logger : loggers) {
             mockAppenders.compute(logger, (k, v) -> {
                 if (v == null) {
                     v = new CopyOnWriteArrayList<>();
                 }
-                v.add(this);
+                v.add(appender);
                 return v;
             });
         }
-        return () -> {
-            isAlive = false;
-            for (String logger : loggers) {
-                mockAppenders.compute(logger, (k, v) -> {
-                    assert v != null;
-                    v.remove(this);
-                    return v.isEmpty() ? null : v;
-                });
-            }
-            // check that all expectations have been evaluated before this is released
-            for (WrappedLoggingExpectation expectation : expectations) {
-                assertThat(
-                    "Method assertMatched() not called on LoggingExpectation instance before release: " + expectation,
-                    expectation.assertMatchedCalled,
-                    is(true)
-                );
-            }
-        };
     }
 
     /**
      * Executes an action and verifies expectations against the provided logger
      */
     public static void assertThatLogger(Runnable action, Class<?> loggerOwner, MockLogAppender.LoggingExpectation expectation) {
-        MockLogAppender mockAppender = new MockLogAppender();
+        try (var mockAppender = MockLogAppender.capture(loggerOwner)) {
-        try (var ignored = mockAppender.capturing(loggerOwner)) {
             mockAppender.addExpectation(expectation);
             action.run();
             mockAppender.assertAllExpectationsMatched();

test/framework/src/main/java/org/elasticsearch/transport/AbstractSimpleTransportTestCase.java

@@ -1319,8 +1319,7 @@ public abstract class AbstractSimpleTransportTestCase extends ESTestCase {
                 .build()
         );
 
-        MockLogAppender appender = new MockLogAppender();
+        try (var appender = MockLogAppender.capture("org.elasticsearch.transport.TransportService.tracer")) {
-        try (var ignored = appender.capturing("org.elasticsearch.transport.TransportService.tracer")) {
 
             ////////////////////////////////////////////////////////////////////////
             // tests for included action type "internal:test"