fix LOGBACK-1672 and LOGBACK-1678

Signed-off-by: Ceki Gulcu <ceki@qos.ch>
qos-ch · Sep 15, 2022 · 717ce71 · 717ce71
1 parent 708c6b1
commit 717ce71
Show file tree

Hide file tree

Showing 8 changed files with 97 additions and 19 deletions.
diff --git a/logback-classic/src/test/input/joran/issues/logback_1162.xml b/logback-classic/src/test/input/joran/issues/logback_1162.xml
@@ -1,5 +1,4 @@
 <configuration debug="false">
-    <statusListener class="ch.qos.logback.core.status.OnConsoleStatusListener" />
 
     <appender name="FILE" class="ch.qos.logback.core.rolling.RollingFileAppender">
         <file>${output_dir}/info.log</file>

diff --git a/logback-classic/src/test/input/joran/issues/logback_1672.xml b/logback-classic/src/test/input/joran/issues/logback_1672.xml
@@ -0,0 +1,12 @@
+<configuration debug="false">
+    <shutdownHook>
+        <delay>10</delay>
+    </shutdownHook>
+
+    <appender name="A" class="ch.qos.logback.core.read.ListAppender"/>
+
+    <root level="DEBUG">
+        <appender-ref ref="A" />
+    </root>
+
+</configuration>
diff --git a/logback-classic/src/test/input/joran/issues/logback_1678.xml b/logback-classic/src/test/input/joran/issues/logback_1678.xml
@@ -0,0 +1,13 @@
+<configuration debug="false">
+
+    <shutdownHook class="ch.qos.logback.core.hook.DelayingShutdownHook">
+        <delay>10</delay>
+    </shutdownHook>
+
+    <appender name="A" class="ch.qos.logback.core.read.ListAppender"/>
+
+    <root level="DEBUG">
+        <appender-ref ref="A" />
+    </root>
+
+</configuration>
diff --git a/logback-classic/src/test/java/ch/qos/logback/classic/joran/JoranConfiguratorTest.java b/logback-classic/src/test/java/ch/qos/logback/classic/joran/JoranConfiguratorTest.java
@@ -13,6 +13,8 @@
  */
 package ch.qos.logback.classic.joran;
 
+import static ch.qos.logback.core.model.processor.ImplicitModelHandler.IGNORING_UNKNOWN_PROP;
+import static ch.qos.logback.core.model.processor.ShutdownHookModelHandler.RENAME_WARNING;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
@@ -23,6 +25,9 @@
 import java.text.SimpleDateFormat;
 import java.util.Date;
 
+import ch.qos.logback.core.model.ShutdownHookModel;
+import ch.qos.logback.core.model.processor.ImplicitModelHandler;
+import ch.qos.logback.core.model.processor.ShutdownHookModelHandler;
 import org.junit.Ignore;
 import org.junit.Test;
 import org.slf4j.MDC;
@@ -237,7 +242,7 @@ public void missingConfigurationElement() throws JoranException {
     public void ignoreUnknownProperty() throws JoranException {
 
         configure(ClassicTestConstants.JORAN_INPUT_PREFIX + "ossfuzz/unknownProperty.xml");
-        String msg = "Ignoring unkown property \\[a\\] in \\[ch.qos.logback.classic.LoggerContext\\]";
+        String msg = IGNORING_UNKNOWN_PROP+" \\[a\\] in \\[ch.qos.logback.classic.LoggerContext\\]";
         checker.assertContainsMatch(Status.WARN, msg);
     }
 
@@ -486,7 +491,7 @@ public void unreferencedAppenderShouldNotTriggerUnknownPropertyMessages() throws
         configure(configFileAsStr);
         checker.assertContainsMatch(Status.WARN,
                 "Appender named \\[EMAIL\\] not referenced. Skipping further processing.");
-        checker.assertNoMatch("Ignoring unkown property \\[evaluator\\]");
+        checker.assertNoMatch(IGNORING_UNKNOWN_PROP+" \\[evaluator\\]");
     }
 
     @Test
@@ -570,7 +575,29 @@ public void shutdownHookTest() throws JoranException {
         String configFileAsStr = ClassicTestConstants.JORAN_INPUT_PREFIX + "issues/logback_1162.xml";
         loggerContext.putProperty("output_dir", ClassicTestConstants.OUTPUT_DIR_PREFIX + "logback_issue_1162/");
         configure(configFileAsStr);
-        assertNotNull(loggerContext.getObject(CoreConstants.SHUTDOWN_HOOK_THREAD));
+        Thread thread = (Thread) loggerContext.getObject(CoreConstants.SHUTDOWN_HOOK_THREAD);
+        assertNotNull(thread);
+    }
+
+
+    @Test
+    public void shutdownHookWithDelayParameter() throws JoranException {
+        String configFileAsStr = ClassicTestConstants.JORAN_INPUT_PREFIX + "issues/logback_1672.xml";
+        configure(configFileAsStr);
+
+        Thread thread = (Thread) loggerContext.getObject(CoreConstants.SHUTDOWN_HOOK_THREAD);
+        assertNotNull(thread);
+        checker.assertNoMatch(IGNORING_UNKNOWN_PROP);
+    }
+
+    @Test
+    public void migrateShutdownHookClassName() throws JoranException {
+        String configFileAsStr = ClassicTestConstants.JORAN_INPUT_PREFIX + "issues/logback_1678.xml";
+        configure(configFileAsStr);
+
+        Thread thread = (Thread) loggerContext.getObject(CoreConstants.SHUTDOWN_HOOK_THREAD);
+        assertNotNull(thread);
+        checker.assertContainsMatch(RENAME_WARNING);
     }
 
     @Test
@@ -626,7 +653,7 @@ public void missingPropertyErrorHandling() throws JoranException {
         assertNotNull(listAppender);
         assertTrue(listAppender.isStarted());
         checker.assertContainsMatch(Status.WARN,
-                "Ignoring unkown property \\[inexistent\\] in \\[ch.qos.logback.core.read.ListAppender\\]");
+                IGNORING_UNKNOWN_PROP+" \\[inexistent\\] in \\[ch.qos.logback.core.read.ListAppender\\]");
     }
 
     @Test

diff --git a/logback-core/src/main/java/ch/qos/logback/core/hook/DefaultShutdownHook.java b/logback-core/src/main/java/ch/qos/logback/core/hook/DefaultShutdownHook.java
@@ -61,4 +61,5 @@ public void run() {
         }
         super.stop();
     }
+
 }
diff --git a/logback-core/src/main/java/ch/qos/logback/core/hook/ShutdownHookBase.java b/logback-core/src/main/java/ch/qos/logback/core/hook/ShutdownHookBase.java
@@ -15,6 +15,7 @@
 
 import ch.qos.logback.core.Context;
 import ch.qos.logback.core.ContextBase;
+import ch.qos.logback.core.CoreConstants;
 import ch.qos.logback.core.spi.ContextAwareBase;
 
 /**
@@ -39,4 +40,5 @@ protected void stop() {
             context.stop();
         }
     }
+
 }
diff --git a/logback-core/src/main/java/ch/qos/logback/core/model/processor/ImplicitModelHandler.java b/logback-core/src/main/java/ch/qos/logback/core/model/processor/ImplicitModelHandler.java
@@ -35,6 +35,7 @@ public class ImplicitModelHandler extends ModelHandlerBase {
     private ImplicitModelData implicitModelData;
 
     static final String PARENT_PROPPERTY_KEY = "parent";
+    static public final String IGNORING_UNKNOWN_PROP = "Ignoring unknown property";
 
     boolean inError = false;
 
@@ -70,11 +71,9 @@ public void handle(ModelInterpretationContext mic, Model model) {
 
         AggregationType aggregationType = parentBean.computeAggregationType(nestedElementTagName);
 
-//        Stack<ImplicitModelData> actionDataStack = mic.getImplicitModelDataStack();
-
         switch (aggregationType) {
         case NOT_FOUND:
-            addWarn("Ignoring unkown property [" + nestedElementTagName + "] in [" + o.getClass().getName() + "]");
+            addWarn(IGNORING_UNKNOWN_PROP+" [" + nestedElementTagName + "] in [" + o.getClass().getName() + "]");
             inError = true;
             // no point in processing submodels
             implicitModel.markAsSkipped();

diff --git a/logback-core/src/main/java/ch/qos/logback/core/model/processor/ShutdownHookModelHandler.java b/logback-core/src/main/java/ch/qos/logback/core/model/processor/ShutdownHookModelHandler.java
@@ -16,6 +16,7 @@
 import ch.qos.logback.core.Context;
 import ch.qos.logback.core.CoreConstants;
 import ch.qos.logback.core.hook.DefaultShutdownHook;
+import ch.qos.logback.core.hook.ShutdownHook;
 import ch.qos.logback.core.hook.ShutdownHookBase;
 import ch.qos.logback.core.model.Model;
 import ch.qos.logback.core.model.ShutdownHookModel;
@@ -25,11 +26,17 @@
 
 public class ShutdownHookModelHandler extends ModelHandlerBase {
 
+    static final String OLD_SHUTDOWN_HOOK_CLASSNAME = "ch.qos.logback.core.hook.DelayingShutdownHook";
+    static final String DEFAULT_SHUTDOWN_HOOK_CLASSNAME = DefaultShutdownHook.class.getName();
+    static public final String RENAME_WARNING = OLD_SHUTDOWN_HOOK_CLASSNAME + " was renamed as "+ DEFAULT_SHUTDOWN_HOOK_CLASSNAME;
+
     public ShutdownHookModelHandler(Context context) {
         super(context);
     }
+    boolean inError = false;
+    ShutdownHook  hook = null;
 
-    static public ModelHandlerBase makeInstance(Context context, ModelInterpretationContext ic) {
+    static public ModelHandlerBase makeInstance(Context context, ModelInterpretationContext mic) {
         return new ShutdownHookModelHandler(context);
     }
 
@@ -39,35 +46,53 @@ protected Class<ShutdownHookModel> getSupportedModelClass() {
     }
 
     @Override
-    public void handle(ModelInterpretationContext interpretationContext, Model model) {
+    public void handle(ModelInterpretationContext mic, Model model) {
 
         ShutdownHookModel shutdownHookModel = (ShutdownHookModel) model;
 
         String className = shutdownHookModel.getClassName();
         if (OptionHelper.isNullOrEmpty(className)) {
-            className = DefaultShutdownHook.class.getName();
+            className = DEFAULT_SHUTDOWN_HOOK_CLASSNAME;
             addInfo("Assuming className [" + className + "]");
         } else {
-            className = interpretationContext.getImport(className);
+            className = mic.getImport(className);
+            if(className.equals(OLD_SHUTDOWN_HOOK_CLASSNAME)) {
+                className = DEFAULT_SHUTDOWN_HOOK_CLASSNAME;
+                addWarn(RENAME_WARNING);
+                addWarn("Please use the new class name");
+            }
         }
 
         addInfo("About to instantiate shutdown hook of type [" + className + "]");
-        ShutdownHookBase hook = null;
+
         try {
             hook = (ShutdownHookBase) OptionHelper.instantiateByClassName(className, ShutdownHookBase.class, context);
             hook.setContext(context);
         } catch (IncompatibleClassException | DynamicClassLoadingException e) {
             addError("Could not create a shutdown hook of type [" + className + "].", e);
+            inError = true;
+            return;
         }
 
-        if (hook == null)
+        mic.pushObject(hook);
+    }
+
+    @Override
+    public void postHandle(ModelInterpretationContext mic, Model model) throws ModelHandlerException {
+        if (inError) {
             return;
+        }
+        Object o = mic.peekObject();
 
-        Thread hookThread = new Thread(hook, "Logback shutdown hook [" + context.getName() + "]");
-        addInfo("Registeting shuthown hook with JVM runtime.");
-        context.putObject(CoreConstants.SHUTDOWN_HOOK_THREAD, hookThread);
-        Runtime.getRuntime().addShutdownHook(hookThread);
+        if (o != hook) {
+            addWarn("The object on the top the of the stack is not the hook object pushed earlier.");
+        } else {
+            Thread hookThread = new Thread(hook, "Logback shutdown hook [" + context.getName() + "]");
+            addInfo("Registering shutdown hook with JVM runtime.");
+            context.putObject(CoreConstants.SHUTDOWN_HOOK_THREAD, hookThread);
+            Runtime.getRuntime().addShutdownHook(hookThread);
 
+            mic.popObject();
+        }
     }
-
 }