Do not stop MockLogAppender instances

When JUnit tests run concurrently they are mostly isolated but they do
share static state. Tests generally don't rely on static state, with the
notable exception of when they manipulate the logger to test the logging
behavior of the code under test. This leads to problems if say test 1
stops a test log appender, then test 2 causes a logging message which
attempts to append to that logging appender. The logging system will
generate an error which leads to spurious failures in test 2. I
previously attempted to avoid these problems by ensuring that test log
appenders are removed before they are stopped. However, that doesn't
completely eliminate race conditions as the list of appenders maintained
by the logging system is essentially a copy-on-write array list. That
means test 2 can get a reference of the appender list, then test 1 can
remove and stop an appended, but test 2 still has a reference to a
version of the list that contained the now stopped appender. I have now
come to the conclusion that the easiest way to fix this problem (without
mucking with the implementation details or concurrency properties of the
logging system itself) is to not stop the test appenders. They maintain
no external state so they can be safely removed without being stopped
and they will just be garbage collected and everyone will be happy.

Signed-off-by: Andrew Ross <andrross@amazon.com>

server/src/test/java/org/opensearch/common/logging/MockAppender.java

@@ -33,15 +33,16 @@
 package org.opensearch.common.logging;
 
 import org.apache.logging.log4j.core.LogEvent;
-import org.apache.logging.log4j.core.appender.AbstractAppender;
+import org.apache.logging.log4j.core.config.Property;
 import org.apache.logging.log4j.core.filter.RegexFilter;
 import org.apache.logging.log4j.message.ParameterizedMessage;
+import org.opensearch.test.AbstractTestAppender;
 
-public class MockAppender extends AbstractAppender {
+public class MockAppender extends AbstractTestAppender {
     public LogEvent lastEvent;
 
     public MockAppender(final String name) throws IllegalAccessException {
-        super(name, RegexFilter.createFilter(".*(\n.*)*", new String[0], false, null, null), null);
+        super(name, RegexFilter.createFilter(".*(\n.*)*", new String[0], false, null, null), null, true, Property.EMPTY_ARRAY);
     }
 
     @Override

server/src/test/java/org/opensearch/common/settings/InsecureSettingTests.java

@@ -13,6 +13,7 @@
 import org.apache.logging.log4j.core.appender.AbstractAppender;
 import org.apache.logging.log4j.core.config.Property;
 import org.opensearch.common.logging.Loggers;
+import org.opensearch.test.AbstractTestAppender;
 import org.opensearch.test.OpenSearchTestCase;
 import org.junit.After;
 import org.junit.Assert;
@@ -43,7 +44,7 @@ private void assertSettingWarning() {
     @Before
     public void addInsecureSettingsAppender() {
         this.rootLogMsgs.clear();
-        rootAppender = new AbstractAppender("root", null, null, true, Property.EMPTY_ARRAY) {
+        rootAppender = new AbstractTestAppender("root", null, null, true, Property.EMPTY_ARRAY) {
             @Override
             public void append(LogEvent event) {
                 String message = event.getMessage().getFormattedMessage();

test/framework/src/main/java/org/opensearch/test/AbstractTestAppender.java

@@ -0,0 +1,43 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+
+package org.opensearch.test;
+
+import org.apache.logging.log4j.core.Filter;
+import org.apache.logging.log4j.core.Layout;
+import org.apache.logging.log4j.core.appender.AbstractAppender;
+import org.apache.logging.log4j.core.config.Property;
+
+import java.io.Serializable;
+
+/**
+ * Extension of {@link AbstractAppender} that provides a no-op stop()
+ * implementation to avoid interference issues between tests running
+ * concurrently.
+ */
+public abstract class AbstractTestAppender extends AbstractAppender {
+
+    protected AbstractTestAppender(
+        String name,
+        Filter filter,
+        Layout<? extends Serializable> layout,
+        boolean ignoreExceptions,
+        Property[] properties
+    ) {
+        super(name, filter, layout, ignoreExceptions, properties);
+    }
+
+    @Override
+    public void stop() {
+        // Do nothing. This feels wrong but there is nothing for this logger
+        // to clean up or flush, and calling the parent stop() will change its
+        // lifecycle state which can lead to race conditions in the static
+        // logger state where in flight logger messages try to append to a
+        // stopped appender and cause spurious test failures.
+    }
+}

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

@@ -34,7 +34,6 @@
 import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.core.LogEvent;
-import org.apache.logging.log4j.core.appender.AbstractAppender;
 import org.apache.logging.log4j.core.config.Property;
 import org.apache.logging.log4j.core.filter.RegexFilter;
 import org.opensearch.common.logging.Loggers;
@@ -52,7 +51,7 @@
 /**
  * Test appender that can be used to verify that certain events were logged correctly
  */
-public class MockLogAppender extends AbstractAppender implements AutoCloseable {
+public class MockLogAppender extends AbstractTestAppender implements AutoCloseable {
 
     private static final String COMMON_PREFIX = System.getProperty("opensearch.logger.prefix", "org.opensearch.");
 
@@ -125,14 +124,7 @@ public void close() {
         for (Logger logger : loggers) {
             Loggers.removeAppender(logger, this);
         }
-        super.stop();
-    }
-
-    @Override
-    public void stop() {
-        // MockLogAppender should be used with try-with-resources to ensure
-        // proper clean up ordering and should never be stopped directly.
-        throw new UnsupportedOperationException("Use close() to ensure proper clean up ordering");
+        stop();
     }
 
     public interface LoggingExpectation {