Fix log appender in InsecureSettingTests (opensearch-project#11866)

The appender was being added to the logger before the appender was started. This can lead to logger errors with concurrently running tests because the logger is static state shared across the JVM. See opensearch-project#10799. I've also added a removeAppender call that was missing from LoggersTests, but that is mostly hygiene and would not lead to failures. Signed-off-by: Andrew Ross <[email protected]>
rayshrey · Mar 18, 2024 · 64f1a2c · 64f1a2c
1 parent 6508cb1
commit 64f1a2c
Show file tree

Hide file tree

Showing 2 changed files with 38 additions and 32 deletions.
diff --git a/server/src/test/java/org/opensearch/common/logging/LoggersTests.java b/server/src/test/java/org/opensearch/common/logging/LoggersTests.java
@@ -53,40 +53,45 @@ public void testParameterizedMessageLambda() throws Exception {
         appender.start();
         final Logger testLogger = LogManager.getLogger(LoggersTests.class);
         Loggers.addAppender(testLogger, appender);
-        Loggers.setLevel(testLogger, Level.TRACE);
+        try {
+            Loggers.setLevel(testLogger, Level.TRACE);
 
-        Throwable ex = randomException();
-        testLogger.error(() -> new ParameterizedMessage("an error message"), ex);
-        assertThat(appender.lastEvent.getLevel(), equalTo(Level.ERROR));
-        assertThat(appender.lastEvent.getThrown(), equalTo(ex));
-        assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("an error message"));
+            Throwable ex = randomException();
+            testLogger.error(() -> new ParameterizedMessage("an error message"), ex);
+            assertThat(appender.lastEvent.getLevel(), equalTo(Level.ERROR));
+            assertThat(appender.lastEvent.getThrown(), equalTo(ex));
+            assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("an error message"));
 
-        ex = randomException();
-        testLogger.warn(() -> new ParameterizedMessage("a warn message: [{}]", "long gc"), ex);
-        assertThat(appender.lastEvent.getLevel(), equalTo(Level.WARN));
-        assertThat(appender.lastEvent.getThrown(), equalTo(ex));
-        assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("a warn message: [long gc]"));
-        assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining("long gc"));
+            ex = randomException();
+            testLogger.warn(() -> new ParameterizedMessage("a warn message: [{}]", "long gc"), ex);
+            assertThat(appender.lastEvent.getLevel(), equalTo(Level.WARN));
+            assertThat(appender.lastEvent.getThrown(), equalTo(ex));
+            assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("a warn message: [long gc]"));
+            assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining("long gc"));
 
-        testLogger.info(() -> new ParameterizedMessage("an info message a=[{}], b=[{}], c=[{}]", 1, 2, 3));
-        assertThat(appender.lastEvent.getLevel(), equalTo(Level.INFO));
-        assertThat(appender.lastEvent.getThrown(), nullValue());
-        assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("an info message a=[1], b=[2], c=[3]"));
-        assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining(1, 2, 3));
+            testLogger.info(() -> new ParameterizedMessage("an info message a=[{}], b=[{}], c=[{}]", 1, 2, 3));
+            assertThat(appender.lastEvent.getLevel(), equalTo(Level.INFO));
+            assertThat(appender.lastEvent.getThrown(), nullValue());
+            assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("an info message a=[1], b=[2], c=[3]"));
+            assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining(1, 2, 3));
 
-        ex = randomException();
-        testLogger.debug(() -> new ParameterizedMessage("a debug message options = {}", Arrays.asList("yes", "no")), ex);
-        assertThat(appender.lastEvent.getLevel(), equalTo(Level.DEBUG));
-        assertThat(appender.lastEvent.getThrown(), equalTo(ex));
-        assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("a debug message options = [yes, no]"));
-        assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining(Arrays.asList("yes", "no")));
+            ex = randomException();
+            testLogger.debug(() -> new ParameterizedMessage("a debug message options = {}", Arrays.asList("yes", "no")), ex);
+            assertThat(appender.lastEvent.getLevel(), equalTo(Level.DEBUG));
+            assertThat(appender.lastEvent.getThrown(), equalTo(ex));
+            assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("a debug message options = [yes, no]"));
+            assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining(Arrays.asList("yes", "no")));
 
-        ex = randomException();
-        testLogger.trace(() -> new ParameterizedMessage("a trace message; element = [{}]", new Object[] { null }), ex);
-        assertThat(appender.lastEvent.getLevel(), equalTo(Level.TRACE));
-        assertThat(appender.lastEvent.getThrown(), equalTo(ex));
-        assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("a trace message; element = [null]"));
-        assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining(new Object[] { null }));
+            ex = randomException();
+            testLogger.trace(() -> new ParameterizedMessage("a trace message; element = [{}]", new Object[] { null }), ex);
+            assertThat(appender.lastEvent.getLevel(), equalTo(Level.TRACE));
+            assertThat(appender.lastEvent.getThrown(), equalTo(ex));
+            assertThat(appender.lastParameterizedMessage().getFormattedMessage(), equalTo("a trace message; element = [null]"));
+            assertThat(appender.lastParameterizedMessage().getParameters(), arrayContaining(new Object[] { null }));
+        } finally {
+            Loggers.removeAppender(testLogger, appender);
+            appender.stop();
+        }
     }
 
     private Throwable randomException() {

diff --git a/server/src/test/java/org/opensearch/common/settings/InsecureSettingTests.java b/server/src/test/java/org/opensearch/common/settings/InsecureSettingTests.java
@@ -25,7 +25,7 @@ public class InsecureSettingTests extends OpenSearchTestCase {
     private List<String> rootLogMsgs = new ArrayList<>();
     private AbstractAppender rootAppender;
 
-    protected void assertSettingWarning() {
+    private void assertSettingWarning() {
         assertWarnings(
             "[setting.name] setting was deprecated in OpenSearch and will be removed in a future release! See the breaking changes documentation for the next major version."
         );
@@ -50,13 +50,14 @@ public void append(LogEvent event) {
                 InsecureSettingTests.this.rootLogMsgs.add(message);
             }
         };
-        Loggers.addAppender(LogManager.getRootLogger(), rootAppender);
         rootAppender.start();
+        Loggers.addAppender(LogManager.getLogger(SecureSetting.class), rootAppender);
     }
 
     @After
     public void removeInsecureSettingsAppender() {
-        Loggers.removeAppender(LogManager.getRootLogger(), rootAppender);
+        Loggers.removeAppender(LogManager.getLogger(SecureSetting.class), rootAppender);
+        rootAppender.stop();
     }
 
     public void testShouldRaiseExceptionByDefault() {