From 9744d282991c8ac4c9e06200a9868d8531469603 Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Wed, 14 Jan 2015 14:37:58 +0000 Subject: [PATCH] =?UTF-8?q?Uninstall=20SLF4J=E2=80=99s=20Java=20logging=20?= =?UTF-8?q?bridge=20handler=20during=20shutdown?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, when LogbackLoggingSystem or Log4JLoggingSystem were initialized during application start up, they would install SLF4J’s Java logging bridge handler, however no corresponding uninstall was performed during application shutdown. When deployed to a servlet container, where the application’s lifecycle doesn’t match the JVM’s lifecycle, this lead to a memory leak. This commit updates LoggingSystem to introduce a new cleanUp method. An empty implementation is provided to preserve backwards compatibility with existing LoggingSystem subclasses. Both LogbackLoggingSystem and Log4JLoggingSystem have been updated to implement cleanUp and uninstall the SLF4J bridge handler. LoggingApplicationListener has been updated to call LoggingSystem.cleanUp in response to a ContextClosedEvent. Closes gh-2324 --- .../logging/LoggingApplicationListener.java | 35 ++++++++++---- .../boot/logging/LoggingSystem.java | 11 ++++- .../logging/log4j/Log4JLoggingSystem.java | 47 +++++++++++++++++-- .../logging/logback/LogbackLoggingSystem.java | 42 ++++++++++++----- .../LoggingApplicationListenerTests.java | 25 +++++++++- .../log4j/Log4JLoggingSystemTests.java | 29 +++++++++++- .../logback/LogbackLoggingSystemTests.java | 26 +++++++++- 7 files changed, 185 insertions(+), 30 deletions(-) diff --git a/spring-boot/src/main/java/org/springframework/boot/logging/LoggingApplicationListener.java b/spring-boot/src/main/java/org/springframework/boot/logging/LoggingApplicationListener.java index 29f56931d6e..2f6ae29e7cc 100644 --- a/spring-boot/src/main/java/org/springframework/boot/logging/LoggingApplicationListener.java +++ b/spring-boot/src/main/java/org/springframework/boot/logging/LoggingApplicationListener.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2014 the original author or authors. + * Copyright 2012-2015 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -28,8 +28,10 @@ import org.springframework.boot.SpringApplication; import org.springframework.boot.bind.RelaxedPropertyResolver; import org.springframework.boot.context.event.ApplicationEnvironmentPreparedEvent; import org.springframework.boot.context.event.ApplicationStartedEvent; +import org.springframework.context.ApplicationContext; import org.springframework.context.ApplicationEvent; import org.springframework.context.ApplicationListener; +import org.springframework.context.event.ContextClosedEvent; import org.springframework.context.event.SmartApplicationListener; import org.springframework.core.Ordered; import org.springframework.core.env.ConfigurableEnvironment; @@ -68,6 +70,7 @@ import org.springframework.util.StringUtils; * * @author Dave Syer * @author Phillip Webb + * @author Andy Wilkinson */ public class LoggingApplicationListener implements SmartApplicationListener { @@ -115,7 +118,10 @@ public class LoggingApplicationListener implements SmartApplicationListener { } private static Class[] EVENT_TYPES = { ApplicationStartedEvent.class, - ApplicationEnvironmentPreparedEvent.class }; + ApplicationEnvironmentPreparedEvent.class, ContextClosedEvent.class }; + + private static Class[] SOURCE_TYPES = { SpringApplication.class, + ApplicationContext.class }; private final Log logger = LogFactory.getLog(getClass()); @@ -127,17 +133,21 @@ public class LoggingApplicationListener implements SmartApplicationListener { @Override public boolean supportsEventType(Class eventType) { - for (Class type : EVENT_TYPES) { - if (type.isAssignableFrom(eventType)) { - return true; - } - } - return false; + return isAssignableFrom(eventType, EVENT_TYPES); } @Override public boolean supportsSourceType(Class sourceType) { - return SpringApplication.class.isAssignableFrom(sourceType); + return isAssignableFrom(sourceType, SOURCE_TYPES); + } + + private boolean isAssignableFrom(Class type, Class[] supportedTypes) { + for (Class supportedType : supportedTypes) { + if (supportedType.isAssignableFrom(type)) { + return true; + } + } + return false; } @Override @@ -147,7 +157,7 @@ public class LoggingApplicationListener implements SmartApplicationListener { initialize(available.getEnvironment(), available.getSpringApplication() .getClassLoader()); } - else { + else if (event instanceof ApplicationStartedEvent) { if (System.getProperty(PID_KEY) == null) { System.setProperty(PID_KEY, new ApplicationPid().toString()); } @@ -155,6 +165,11 @@ public class LoggingApplicationListener implements SmartApplicationListener { .getDefaultClassLoader()); loggingSystem.beforeInitialize(); } + else { + LoggingSystem loggingSystem = LoggingSystem.get(ClassUtils + .getDefaultClassLoader()); + loggingSystem.cleanUp(); + } } /** diff --git a/spring-boot/src/main/java/org/springframework/boot/logging/LoggingSystem.java b/spring-boot/src/main/java/org/springframework/boot/logging/LoggingSystem.java index 469ec1dde38..4749609bf6c 100644 --- a/spring-boot/src/main/java/org/springframework/boot/logging/LoggingSystem.java +++ b/spring-boot/src/main/java/org/springframework/boot/logging/LoggingSystem.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2013 the original author or authors. + * Copyright 2012-2015 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -27,6 +27,7 @@ import org.springframework.util.ClassUtils; * * @author Phillip Webb * @author Dave Syer + * @author Andy Wilkinson */ public abstract class LoggingSystem { @@ -61,6 +62,14 @@ public abstract class LoggingSystem { */ public abstract void initialize(String configLocation); + /** + * Clean up the logging system. The default implementation does nothing. Subclasses + * should override this method to perform any logging system-specific cleanup. + */ + public void cleanUp() { + + } + /** * Sets the logging level for a given logger. * @param loggerName the name of the logger to set 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 bd7a83bec6b..1fed364f028 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 @@ -1,5 +1,5 @@ /* - * Copyright 2012-2013 the original author or authors. + * Copyright 2012-2015 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -37,6 +37,7 @@ import org.springframework.util.StringUtils; * * @author Phillip Webb * @author Dave Syer + * @author Andy Wilkinson */ public class Log4JLoggingSystem extends AbstractLoggingSystem { @@ -60,10 +61,7 @@ public class Log4JLoggingSystem extends AbstractLoggingSystem { @Override public void beforeInitialize() { super.beforeInitialize(); - if (ClassUtils.isPresent("org.slf4j.bridge.SLF4JBridgeHandler", getClassLoader())) { - SLF4JBridgeHandler.removeHandlersForRootLogger(); - SLF4JBridgeHandler.install(); - } + configureJdkLoggingBridgeHandler(); } @Override @@ -78,6 +76,45 @@ public class Log4JLoggingSystem extends AbstractLoggingSystem { } } + @Override + public void cleanUp() { + removeJdkLoggingBridgeHandler(); + } + + private void configureJdkLoggingBridgeHandler() { + try { + if (bridgeHandlerIsAvailable()) { + removeJdkLoggingBridgeHandler(); + SLF4JBridgeHandler.install(); + } + } + catch (Throwable ex) { + // Ignore. No java.util.logging bridge is installed. + } + } + + private boolean bridgeHandlerIsAvailable() { + return ClassUtils.isPresent("org.slf4j.bridge.SLF4JBridgeHandler", + getClassLoader()); + } + + private void removeJdkLoggingBridgeHandler() { + try { + if (bridgeHandlerIsAvailable()) { + try { + SLF4JBridgeHandler.removeHandlersForRootLogger(); + } + catch (NoSuchMethodError ex) { + // Method missing in older versions of SLF4J like in JBoss AS 7.1 + SLF4JBridgeHandler.uninstall(); + } + } + } + catch (Throwable ex) { + // Ignore and continue + } + } + @Override public void setLogLevel(String loggerName, LogLevel level) { Logger logger = (StringUtils.hasLength(loggerName) ? LogManager 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 d6338397eaf..fce8b0e7dbc 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 @@ -1,5 +1,5 @@ /* - * Copyright 2012-2014 the original author or authors. + * Copyright 2012-2015 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -41,7 +41,7 @@ import ch.qos.logback.classic.LoggerContext; import ch.qos.logback.classic.util.ContextInitializer; /** - * {@link LoggingSystem} for for logback. + * {@link LoggingSystem} for logback. * * @author Phillip Webb * @author Dave Syer @@ -74,17 +74,15 @@ public class LogbackLoggingSystem extends AbstractLoggingSystem { configureJBossLoggingToUseSlf4j(); } + @Override + public void cleanUp() { + removeJdkLoggingBridgeHandler(); + } + private void configureJdkLoggingBridgeHandler() { try { - if (ClassUtils.isPresent("org.slf4j.bridge.SLF4JBridgeHandler", - getClassLoader())) { - try { - SLF4JBridgeHandler.removeHandlersForRootLogger(); - } - catch (NoSuchMethodError ex) { - // Method missing in older versions of SLF4J like in JBoss AS 7.1 - SLF4JBridgeHandler.uninstall(); - } + if (bridgeHandlerIsAvailable()) { + removeJdkLoggingBridgeHandler(); SLF4JBridgeHandler.install(); } } @@ -93,10 +91,32 @@ public class LogbackLoggingSystem extends AbstractLoggingSystem { } } + private boolean bridgeHandlerIsAvailable() { + return ClassUtils.isPresent("org.slf4j.bridge.SLF4JBridgeHandler", + getClassLoader()); + } + private void configureJBossLoggingToUseSlf4j() { System.setProperty("org.jboss.logging.provider", "slf4j"); } + private void removeJdkLoggingBridgeHandler() { + try { + if (bridgeHandlerIsAvailable()) { + try { + SLF4JBridgeHandler.removeHandlersForRootLogger(); + } + catch (NoSuchMethodError ex) { + // Method missing in older versions of SLF4J like in JBoss AS 7.1 + SLF4JBridgeHandler.uninstall(); + } + } + } + catch (Throwable ex) { + // Ignore and continue + } + } + @Override public void initialize(String configLocation) { Assert.notNull(configLocation, "ConfigLocation must not be null"); diff --git a/spring-boot/src/test/java/org/springframework/boot/logging/LoggingApplicationListenerTests.java b/spring-boot/src/test/java/org/springframework/boot/logging/LoggingApplicationListenerTests.java index eb871fa1dce..5ce26cac60a 100644 --- a/spring-boot/src/test/java/org/springframework/boot/logging/LoggingApplicationListenerTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/logging/LoggingApplicationListenerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2014 the original author or authors. + * Copyright 2012-2015 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -18,7 +18,9 @@ package org.springframework.boot.logging; import java.io.File; import java.io.IOException; +import java.util.logging.Handler; import java.util.logging.LogManager; +import java.util.logging.Logger; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -28,11 +30,13 @@ import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; +import org.slf4j.bridge.SLF4JBridgeHandler; import org.springframework.boot.SpringApplication; import org.springframework.boot.context.event.ApplicationStartedEvent; import org.springframework.boot.logging.java.JavaLoggingSystem; import org.springframework.boot.test.EnvironmentTestUtils; import org.springframework.boot.test.OutputCapture; +import org.springframework.context.event.ContextClosedEvent; import org.springframework.context.support.GenericApplicationContext; import static org.hamcrest.Matchers.containsString; @@ -46,6 +50,7 @@ import static org.junit.Assert.assertTrue; * * @author Dave Syer * @author Phillip Webb + * @author Andy Wilkinson */ public class LoggingApplicationListenerTests { @@ -261,4 +266,22 @@ public class LoggingApplicationListenerTests { this.logger.debug("testatdebug"); assertThat(this.outputCapture.toString(), not(containsString("testatdebug"))); } + + @Test + public void bridgeHandlerLifecycle() throws Exception { + assertTrue(bridgeHandlerInstalled()); + this.initializer.onApplicationEvent(new ContextClosedEvent(this.context)); + assertFalse(bridgeHandlerInstalled()); + } + + private boolean bridgeHandlerInstalled() { + Logger rootLogger = LogManager.getLogManager().getLogger(""); + Handler[] handlers = rootLogger.getHandlers(); + for (Handler handler : handlers) { + if (handler instanceof SLF4JBridgeHandler) { + return true; + } + } + return false; + } } 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 8fdc52b1a12..e4305511f6a 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 @@ -1,5 +1,5 @@ /* - * Copyright 2012-2014 the original author or authors. + * Copyright 2012-2015 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,16 +16,21 @@ package org.springframework.boot.logging.log4j; +import java.util.logging.Handler; +import java.util.logging.LogManager; + import org.apache.commons.logging.impl.Log4JLogger; import org.junit.After; import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.slf4j.bridge.SLF4JBridgeHandler; import org.springframework.boot.logging.LogLevel; import org.springframework.boot.test.OutputCapture; import org.springframework.util.StringUtils; import static org.hamcrest.Matchers.equalTo; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; @@ -33,6 +38,7 @@ import static org.junit.Assert.assertTrue; * Tests for {@link Log4JLoggingSystem}. * * @author Phillip Webb + * @author Andy Wilkinson */ public class Log4JLoggingSystemTests { @@ -51,6 +57,7 @@ public class Log4JLoggingSystemTests { @After public void clear() { + this.loggingSystem.cleanUp(); System.clearProperty("LOG_FILE"); System.clearProperty("LOG_PATH"); System.clearProperty("PID"); @@ -89,4 +96,24 @@ public class Log4JLoggingSystemTests { equalTo(1)); } + @Test + public void bridgeHandlerLifecycle() { + assertFalse(bridgeHandlerInstalled()); + this.loggingSystem.beforeInitialize(); + assertTrue(bridgeHandlerInstalled()); + this.loggingSystem.cleanUp(); + assertFalse(bridgeHandlerInstalled()); + } + + private boolean bridgeHandlerInstalled() { + java.util.logging.Logger rootLogger = LogManager.getLogManager().getLogger(""); + Handler[] handlers = rootLogger.getHandlers(); + for (Handler handler : handlers) { + if (handler instanceof SLF4JBridgeHandler) { + return true; + } + } + return false; + } + } 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 331dacf9855..314db8c7daf 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 @@ -1,5 +1,5 @@ /* - * Copyright 2012-2014 the original author or authors. + * Copyright 2012-2015 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -17,6 +17,8 @@ package org.springframework.boot.logging.logback; import java.io.File; +import java.util.logging.Handler; +import java.util.logging.LogManager; import org.apache.commons.logging.Log; import org.apache.commons.logging.impl.SLF4JLogFactory; @@ -25,6 +27,7 @@ import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.slf4j.ILoggerFactory; +import org.slf4j.bridge.SLF4JBridgeHandler; import org.slf4j.impl.StaticLoggerBinder; import org.springframework.boot.logging.LogLevel; import org.springframework.boot.test.OutputCapture; @@ -72,6 +75,7 @@ public class LogbackLoggingSystemTests { @After public void clear() { + this.loggingSystem.cleanUp(); System.clearProperty("LOG_FILE"); System.clearProperty("LOG_PATH"); System.clearProperty("PID"); @@ -127,4 +131,24 @@ public class LogbackLoggingSystemTests { assertEquals("slf4j", System.getProperty("org.jboss.logging.provider")); } + @Test + public void bridgeHandlerLifecycle() { + assertFalse(bridgeHandlerInstalled()); + this.loggingSystem.beforeInitialize(); + assertTrue(bridgeHandlerInstalled()); + this.loggingSystem.cleanUp(); + assertFalse(bridgeHandlerInstalled()); + } + + private boolean bridgeHandlerInstalled() { + java.util.logging.Logger rootLogger = LogManager.getLogManager().getLogger(""); + Handler[] handlers = rootLogger.getHandlers(); + for (Handler handler : handlers) { + if (handler instanceof SLF4JBridgeHandler) { + return true; + } + } + return false; + } + }