From f7e8e1554c69054e2fa53b4a4966078fc802a125 Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Tue, 29 Sep 2015 18:03:02 +0100 Subject: [PATCH] Shut down logging system as part of clean up processing Previously, when a LoggingSystem was cleaned up the underlying logging system (LogBack, Log4J, Log4J2, JUL) was not being properly shut down. LogBack explicitly recommends stopping it cleanly [1]. While the other logging systems are less explicity about it, they would all appear to benefit from being stopped. This commit updates all four LoggingSystems to stop/reset/clean up to underlying logging system when cleanUp is called. Closes gh-4026 [1] http://logback.qos.ch/manual/configuration.html#stopContext --- .../boot/logging/java/JavaLoggingSystem.java | 6 ++++ .../logging/log4j/Log4JLoggingSystem.java | 6 ++++ .../logging/log4j2/Log4J2LoggingSystem.java | 6 ++++ .../logging/logback/LogbackLoggingSystem.java | 1 + .../logging/java/JavaLoggingSystemTests.java | 33 +++++++++++++++++++ .../log4j/Log4JLoggingSystemTests.java | 11 +++++++ .../log4j2/Log4J2LoggingSystemTests.java | 15 +++++++-- .../logback/LogbackLoggingSystemTests.java | 15 +++++++++ 8 files changed, 91 insertions(+), 2 deletions(-) diff --git a/spring-boot/src/main/java/org/springframework/boot/logging/java/JavaLoggingSystem.java b/spring-boot/src/main/java/org/springframework/boot/logging/java/JavaLoggingSystem.java index fe172f0846f..395b3329782 100644 --- a/spring-boot/src/main/java/org/springframework/boot/logging/java/JavaLoggingSystem.java +++ b/spring-boot/src/main/java/org/springframework/boot/logging/java/JavaLoggingSystem.java @@ -113,4 +113,10 @@ public class JavaLoggingSystem extends AbstractLoggingSystem { logger.setLevel(LEVELS.get(level)); } + @Override + public void cleanUp() { + super.cleanUp(); + LogManager.getLogManager().reset(); + } + } diff --git a/spring-boot/src/main/java/org/springframework/boot/logging/log4j/Log4JLoggingSystem.java b/spring-boot/src/main/java/org/springframework/boot/logging/log4j/Log4JLoggingSystem.java index fab44a682ad..380114832ef 100644 --- a/spring-boot/src/main/java/org/springframework/boot/logging/log4j/Log4JLoggingSystem.java +++ b/spring-boot/src/main/java/org/springframework/boot/logging/log4j/Log4JLoggingSystem.java @@ -115,4 +115,10 @@ public class Log4JLoggingSystem extends Slf4JLoggingSystem { logger.setLevel(LEVELS.get(level)); } + @Override + public void cleanUp() { + super.cleanUp(); + LogManager.shutdown(); + } + } diff --git a/spring-boot/src/main/java/org/springframework/boot/logging/log4j2/Log4J2LoggingSystem.java b/spring-boot/src/main/java/org/springframework/boot/logging/log4j2/Log4J2LoggingSystem.java index 4bcc9c5909f..753e7805577 100644 --- a/spring-boot/src/main/java/org/springframework/boot/logging/log4j2/Log4J2LoggingSystem.java +++ b/spring-boot/src/main/java/org/springframework/boot/logging/log4j2/Log4J2LoggingSystem.java @@ -152,6 +152,12 @@ public class Log4J2LoggingSystem extends Slf4JLoggingSystem { loadConfiguration(location, logFile); } + @Override + public void cleanUp() { + super.cleanUp(); + getLoggerContext().stop(); + } + protected void loadConfiguration(String location, LogFile logFile) { Assert.notNull(location, "Location must not be null"); if (logFile != null) { diff --git a/spring-boot/src/main/java/org/springframework/boot/logging/logback/LogbackLoggingSystem.java b/spring-boot/src/main/java/org/springframework/boot/logging/logback/LogbackLoggingSystem.java index 1a12ef86744..57225fe886d 100644 --- a/spring-boot/src/main/java/org/springframework/boot/logging/logback/LogbackLoggingSystem.java +++ b/spring-boot/src/main/java/org/springframework/boot/logging/logback/LogbackLoggingSystem.java @@ -177,6 +177,7 @@ public class LogbackLoggingSystem extends Slf4JLoggingSystem { public void cleanUp() { super.cleanUp(); getLoggerContext().getStatusManager().clear(); + getLoggerContext().stop(); } @Override diff --git a/spring-boot/src/test/java/org/springframework/boot/logging/java/JavaLoggingSystemTests.java b/spring-boot/src/test/java/org/springframework/boot/logging/java/JavaLoggingSystemTests.java index c2bccf3462f..95931ac0539 100644 --- a/spring-boot/src/test/java/org/springframework/boot/logging/java/JavaLoggingSystemTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/logging/java/JavaLoggingSystemTests.java @@ -20,6 +20,9 @@ import java.io.File; import java.io.FileFilter; import java.io.IOException; import java.util.Locale; +import java.util.logging.Handler; +import java.util.logging.LogManager; +import java.util.logging.LogRecord; import org.apache.commons.logging.impl.Jdk14Logger; import org.junit.After; @@ -34,6 +37,7 @@ import org.springframework.util.StringUtils; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThan; +import static org.hamcrest.Matchers.is; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; @@ -160,4 +164,33 @@ public class JavaLoggingSystemTests extends AbstractLoggingSystemTests { equalTo(1)); } + @Test + public void cleanUpResetsLogManager() throws Exception { + this.loggingSystem.beforeInitialize(); + this.loggingSystem.initialize(null, null, null); + this.logger.getLogger().addHandler(new NoOpHandler()); + assertThat(this.logger.getLogger().getHandlers().length, is(equalTo(1))); + LogManager.getLogManager().reset(); + assertThat(this.logger.getLogger().getHandlers().length, is(equalTo(0))); + } + + private static final class NoOpHandler extends Handler { + + @Override + public void publish(LogRecord record) { + + } + + @Override + public void flush() { + + } + + @Override + public void close() throws SecurityException { + + } + + } + } diff --git a/spring-boot/src/test/java/org/springframework/boot/logging/log4j/Log4JLoggingSystemTests.java b/spring-boot/src/test/java/org/springframework/boot/logging/log4j/Log4JLoggingSystemTests.java index 02960d5e86b..5cdc2efbd28 100644 --- a/spring-boot/src/test/java/org/springframework/boot/logging/log4j/Log4JLoggingSystemTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/logging/log4j/Log4JLoggingSystemTests.java @@ -139,6 +139,17 @@ public class Log4JLoggingSystemTests extends AbstractLoggingSystemTests { assertFalse(bridgeHandlerInstalled()); } + @Test + public void cleanUpStopsLogManager() { + this.loggingSystem.beforeInitialize(); + this.loggingSystem.initialize(null, null, null); + assertTrue(org.apache.log4j.LogManager.getLoggerRepository().getRootLogger() + .getAllAppenders().hasMoreElements()); + this.loggingSystem.cleanUp(); + assertFalse(org.apache.log4j.LogManager.getLoggerRepository().getRootLogger() + .getAllAppenders().hasMoreElements()); + } + private boolean bridgeHandlerInstalled() { java.util.logging.Logger rootLogger = LogManager.getLogManager().getLogger(""); Handler[] handlers = rootLogger.getHandlers(); diff --git a/spring-boot/src/test/java/org/springframework/boot/logging/log4j2/Log4J2LoggingSystemTests.java b/spring-boot/src/test/java/org/springframework/boot/logging/log4j2/Log4J2LoggingSystemTests.java index 4b8c11b8918..4ee7fe6d38d 100644 --- a/spring-boot/src/test/java/org/springframework/boot/logging/log4j2/Log4J2LoggingSystemTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/logging/log4j2/Log4J2LoggingSystemTests.java @@ -24,6 +24,7 @@ import java.util.List; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.apache.logging.log4j.core.LoggerContext; import org.apache.logging.log4j.core.config.Configuration; import org.apache.logging.log4j.core.config.FileConfigurationMonitor; import org.hamcrest.Matcher; @@ -238,6 +239,17 @@ public class Log4J2LoggingSystemTests extends AbstractLoggingSystemTests { } } + @Test + public void cleanupStopsContext() throws Exception { + this.loggingSystem.beforeInitialize(); + this.logger.info("Hidden"); + this.loggingSystem.initialize(null, null, null); + LoggerContext context = (LoggerContext) LogManager.getContext(false); + assertFalse(context.isStopped()); + this.loggingSystem.cleanUp(); + assertTrue(context.isStopped()); + } + private static class TestLog4J2LoggingSystem extends Log4J2LoggingSystem { private List availableClasses = new ArrayList(); @@ -247,8 +259,7 @@ public class Log4J2LoggingSystemTests extends AbstractLoggingSystemTests { } public Configuration getConfiguration() { - return ((org.apache.logging.log4j.core.LoggerContext) LogManager - .getContext(false)).getConfiguration(); + return ((LoggerContext) LogManager.getContext(false)).getConfiguration(); } @Override diff --git a/spring-boot/src/test/java/org/springframework/boot/logging/logback/LogbackLoggingSystemTests.java b/spring-boot/src/test/java/org/springframework/boot/logging/logback/LogbackLoggingSystemTests.java index 1db49aece1c..4e21104376f 100644 --- a/spring-boot/src/test/java/org/springframework/boot/logging/logback/LogbackLoggingSystemTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/logging/logback/LogbackLoggingSystemTests.java @@ -51,6 +51,7 @@ import static org.hamcrest.Matchers.not; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; @@ -121,6 +122,7 @@ public class LogbackLoggingSystemTests extends AbstractLoggingSystemTests { @Test public void testBasicConfigLocation() throws Exception { this.loggingSystem.beforeInitialize(); + this.loggingSystem.initialize(this.initializationContext, null, null); ILoggerFactory factory = StaticLoggerBinder.getSingleton().getLoggerFactory(); LoggerContext context = (LoggerContext) factory; Logger root = context.getLogger(org.slf4j.Logger.ROOT_LOGGER_NAME); @@ -305,6 +307,19 @@ public class LogbackLoggingSystemTests extends AbstractLoggingSystemTests { } } + @Test + public void cleanUpStopsContext() throws Exception { + this.loggingSystem.beforeInitialize(); + this.loggingSystem.initialize(this.initializationContext, null, null); + ILoggerFactory factory = StaticLoggerBinder.getSingleton().getLoggerFactory(); + LoggerContext context = (LoggerContext) factory; + Logger root = context.getLogger(org.slf4j.Logger.ROOT_LOGGER_NAME); + assertNotNull(root.getAppender("CONSOLE")); + + this.loggingSystem.cleanUp(); + assertNull(root.getAppender("CONSOLE")); + } + private String getLineWithText(File file, String outputSearch) throws Exception { return getLineWithText(FileCopyUtils.copyToString(new FileReader(file)), outputSearch);