Uninstall SLF4J’s Java logging bridge handler during shutdown

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

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<? extends ApplicationEvent> eventType) {
-		for (Class<?> type : EVENT_TYPES) {
+		return isAssignableFrom(eventType, EVENT_TYPES);
-			if (type.isAssignableFrom(eventType)) {
-				return true;
-			}
-		}
-		return false;
 	}
 
 	@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();
+		}
 	}
 
 	/**

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

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())) {
+		configureJdkLoggingBridgeHandler();
-			SLF4JBridgeHandler.removeHandlersForRootLogger();
-			SLF4JBridgeHandler.install();
-		}
 	}
 
 	@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

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 <a href="http://logback.qos.ch">logback</a>.
+ * {@link LoggingSystem} for <a href="http://logback.qos.ch">logback</a>.
  *
  * @author Phillip Webb
  * @author Dave Syer
@@ -74,10 +74,35 @@ public class LogbackLoggingSystem extends AbstractLoggingSystem {
 		configureJBossLoggingToUseSlf4j();
 	}
 
+	@Override
+	public void cleanUp() {
+		removeJdkLoggingBridgeHandler();
+	}
+
 	private void configureJdkLoggingBridgeHandler() {
 		try {
-			if (ClassUtils.isPresent("org.slf4j.bridge.SLF4JBridgeHandler",
+			if (bridgeHandlerIsAvailable()) {
-					getClassLoader())) {
+				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 configureJBossLoggingToUseSlf4j() {
+		System.setProperty("org.jboss.logging.provider", "slf4j");
+	}
+
+	private void removeJdkLoggingBridgeHandler() {
+		try {
+			if (bridgeHandlerIsAvailable()) {
 				try {
 					SLF4JBridgeHandler.removeHandlersForRootLogger();
 				}
@@ -85,18 +110,13 @@ public class LogbackLoggingSystem extends AbstractLoggingSystem {
 					// Method missing in older versions of SLF4J like in JBoss AS 7.1
 					SLF4JBridgeHandler.uninstall();
 				}
-				SLF4JBridgeHandler.install();
 			}
 		}
 		catch (Throwable ex) {
-			// Ignore. No java.util.logging bridge is installed.
+			// Ignore and continue
 		}
 	}
 
-	private void configureJBossLoggingToUseSlf4j() {
-		System.setProperty("org.jboss.logging.provider", "slf4j");
-	}
-
 	@Override
 	public void initialize(String configLocation) {
 		Assert.notNull(configLocation, "ConfigLocation must not be null");

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;
+	}
 }

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;
+	}
+
 }

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;
+	}
+
 }