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

Author: wilkinsona

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.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 @@
  *
  * @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) {
-			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,14 +157,19 @@ public void onApplicationEvent(ApplicationEvent event) {
 			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());
 			}
 			LoggingSystem loggingSystem = LoggingSystem.get(ClassUtils
 					.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 @@
  *
  * @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 @@
  *
  * @author Phillip Webb
  * @author Dave Syer
+ * @author Andy Wilkinson
  */
 public class Log4JLoggingSystem extends AbstractLoggingSystem {
 
@@ -60,10 +61,7 @@ public Log4JLoggingSystem(ClassLoader classLoader) {
 	@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 void initialize(String configLocation) {
 		}
 	}
 
+	@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.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,29 +74,49 @@ public void beforeInitialize() {
 		configureJBossLoggingToUseSlf4j();
 	}
 
+	@Override
+	public void cleanUp() {
+		removeJdkLoggingBridgeHandler();
+	}
+
 	private void configureJdkLoggingBridgeHandler() {
 		try {
-			if (ClassUtils.isPresent("org.slf4j.bridge.SLF4JBridgeHandler",
-					getClassLoader())) {
+			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 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();
 				}
-				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 @@
 
 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.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 @@
  *
  * @author Dave Syer
  * @author Phillip Webb
+ * @author Andy Wilkinson
  */
 public class LoggingApplicationListenerTests {
 
@@ -261,4 +266,22 @@ public void parseArgsDoesntReplace() throws Exception {
 		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,23 +16,29 @@
 
 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;
 
 /**
  * Tests for {@link Log4JLoggingSystem}.
  *
  * @author Phillip Webb
+ * @author Andy Wilkinson
  */
 public class Log4JLoggingSystemTests {
 
@@ -51,6 +57,7 @@ public void setup() {
 
 	@After
 	public void clear() {
+		this.loggingSystem.cleanUp();
 		System.clearProperty("LOG_FILE");
 		System.clearProperty("LOG_PATH");
 		System.clearProperty("PID");
@@ -89,4 +96,24 @@ public void setLevel() throws Exception {
 				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;
+	}
+
 }