From 8619256d2a425a60235e1e1491e9bb9e5ce5d00d Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Fri, 19 Jan 2018 11:13:17 +0000 Subject: [PATCH] Clean up the logging system later in context close processing Previously, the logging system was cleaned up in response to the root context's ContextClosedEvent being received. This event is published early in a context's close processing. As a result, the logging system is in cleaned up state while, for example, disposable beans are being destroyed. This commit reworks the logic that triggers logging system clean up to use a disposable bean instead. Disposable beans are called in reverse-registration order. The logging clean up bean is registered as early as possible so that it should be the last disposable bean to be called. Closes gh-11676 --- .../BackgroundPreinitializer.java | 4 +- .../devtools/RemoteSpringApplication.java | 6 +- .../ClasspathLoggingApplicationListener.java | 4 +- ...tener.java => LoggingSystemLifecycle.java} | 69 ++++++++++++++----- .../main/resources/META-INF/spring.factories | 3 +- .../SpringApplicationBuilderTests.java | 6 +- ...ggingSystemLifecycleIntegrationTests.java} | 6 +- ....java => LoggingSystemLifecycleTests.java} | 37 +++++----- 8 files changed, 88 insertions(+), 47 deletions(-) rename spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/logging/{LoggingApplicationListener.java => LoggingSystemLifecycle.java} (84%) rename spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/logging/{LoggingApplicationListenerIntegrationTests.java => LoggingSystemLifecycleIntegrationTests.java} (93%) rename spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/logging/{LoggingApplicationListenerTests.java => LoggingSystemLifecycleTests.java} (96%) diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/BackgroundPreinitializer.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/BackgroundPreinitializer.java index 3d4a697224..023adde744 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/BackgroundPreinitializer.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/BackgroundPreinitializer.java @@ -30,7 +30,7 @@ import org.springframework.boot.context.event.ApplicationFailedEvent; import org.springframework.boot.context.event.ApplicationReadyEvent; import org.springframework.boot.context.event.ApplicationStartingEvent; import org.springframework.boot.context.event.SpringApplicationEvent; -import org.springframework.boot.context.logging.LoggingApplicationListener; +import org.springframework.boot.context.logging.LoggingSystemLifecycle; import org.springframework.context.ApplicationListener; import org.springframework.core.annotation.Order; import org.springframework.format.support.DefaultFormattingConversionService; @@ -45,7 +45,7 @@ import org.springframework.http.converter.support.AllEncompassingFormHttpMessage * @author Andy Wilkinson * @since 1.3.0 */ -@Order(LoggingApplicationListener.DEFAULT_ORDER + 1) +@Order(LoggingSystemLifecycle.DEFAULT_ORDER + 1) public class BackgroundPreinitializer implements ApplicationListener { diff --git a/spring-boot-project/spring-boot-devtools/src/main/java/org/springframework/boot/devtools/RemoteSpringApplication.java b/spring-boot-project/spring-boot-devtools/src/main/java/org/springframework/boot/devtools/RemoteSpringApplication.java index 8bdf496f69..f0f2fc30cf 100644 --- a/spring-boot-project/spring-boot-devtools/src/main/java/org/springframework/boot/devtools/RemoteSpringApplication.java +++ b/spring-boot-project/spring-boot-devtools/src/main/java/org/springframework/boot/devtools/RemoteSpringApplication.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2017 the original author or authors. + * Copyright 2012-2018 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,7 +27,7 @@ import org.springframework.boot.WebApplicationType; import org.springframework.boot.context.config.AnsiOutputApplicationListener; import org.springframework.boot.context.config.ConfigFileApplicationListener; import org.springframework.boot.context.logging.ClasspathLoggingApplicationListener; -import org.springframework.boot.context.logging.LoggingApplicationListener; +import org.springframework.boot.context.logging.LoggingSystemLifecycle; import org.springframework.boot.devtools.remote.client.RemoteClientConfiguration; import org.springframework.boot.devtools.restart.RestartInitializer; import org.springframework.boot.devtools.restart.RestartScopeInitializer; @@ -74,7 +74,7 @@ public final class RemoteSpringApplication { listeners.add(new AnsiOutputApplicationListener()); listeners.add(new ConfigFileApplicationListener()); listeners.add(new ClasspathLoggingApplicationListener()); - listeners.add(new LoggingApplicationListener()); + listeners.add(new LoggingSystemLifecycle()); listeners.add(new RemoteUrlPropertyExtractor()); return listeners; } diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/logging/ClasspathLoggingApplicationListener.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/logging/ClasspathLoggingApplicationListener.java index 1f59c21a98..a0f7671555 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/logging/ClasspathLoggingApplicationListener.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/logging/ClasspathLoggingApplicationListener.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2017 the original author or authors. + * Copyright 2012-2018 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 org.springframework.core.ResolvableType; public final class ClasspathLoggingApplicationListener implements GenericApplicationListener { - private static final int ORDER = LoggingApplicationListener.DEFAULT_ORDER + 1; + private static final int ORDER = LoggingSystemLifecycle.DEFAULT_ORDER + 1; private static final Log logger = LogFactory .getLog(ClasspathLoggingApplicationListener.class); diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/logging/LoggingApplicationListener.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/logging/LoggingSystemLifecycle.java similarity index 84% rename from spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/logging/LoggingApplicationListener.java rename to spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/logging/LoggingSystemLifecycle.java index e8aa80382e..979d487d4d 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/logging/LoggingApplicationListener.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/logging/LoggingSystemLifecycle.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2017 the original author or authors. + * Copyright 2012-2018 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. @@ -24,7 +24,12 @@ import java.util.concurrent.atomic.AtomicBoolean; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.beans.factory.BeanFactory; +import org.springframework.beans.factory.DisposableBean; +import org.springframework.beans.factory.config.BeanDefinition; import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; +import org.springframework.beans.factory.support.BeanDefinitionBuilder; +import org.springframework.beans.factory.support.BeanDefinitionRegistry; import org.springframework.boot.SpringApplication; import org.springframework.boot.context.event.ApplicationEnvironmentPreparedEvent; import org.springframework.boot.context.event.ApplicationFailedEvent; @@ -38,8 +43,9 @@ import org.springframework.boot.logging.LoggingInitializationContext; import org.springframework.boot.logging.LoggingSystem; import org.springframework.boot.logging.LoggingSystemProperties; import org.springframework.context.ApplicationContext; +import org.springframework.context.ApplicationContextInitializer; import org.springframework.context.ApplicationEvent; -import org.springframework.context.ApplicationListener; +import org.springframework.context.ConfigurableApplicationContext; import org.springframework.context.event.ContextClosedEvent; import org.springframework.context.event.GenericApplicationListener; import org.springframework.core.Ordered; @@ -52,10 +58,11 @@ import org.springframework.util.ResourceUtils; import org.springframework.util.StringUtils; /** - * An {@link ApplicationListener} that configures the {@link LoggingSystem}. If the - * environment contains a {@code logging.config} property it will be used to bootstrap the - * logging system, otherwise a default configuration is used. Regardless, logging levels - * will be customized if the environment contains {@code logging.level.*} entries. + * {@code LoggingSystemLifecyle} configures the {@link LoggingSystem} and manages its + * lifecycle. If the environment contains a {@code logging.config} property it will be + * used to bootstrap the logging system, otherwise a default configuration is used. + * Regardless, logging levels will be customized if the environment contains + * {@code logging.level.*} entries. *

* Debug and trace logging for Spring, Tomcat, Jetty and Hibernate will be enabled when * the environment contains {@code debug} or {@code trace} properties that aren't set to @@ -82,7 +89,8 @@ import org.springframework.util.StringUtils; * @since 2.0.0 * @see LoggingSystem#get(ClassLoader) */ -public class LoggingApplicationListener implements GenericApplicationListener { +public class LoggingSystemLifecycle implements GenericApplicationListener, + ApplicationContextInitializer, Ordered { private static final Bindable> STRING_STRING_MAP = Bindable .mapOf(String.class, String.class); @@ -175,10 +183,6 @@ public class LoggingApplicationListener implements GenericApplicationListener { else if (event instanceof ApplicationPreparedEvent) { onApplicationPreparedEvent((ApplicationPreparedEvent) event); } - else if (event instanceof ContextClosedEvent && ((ContextClosedEvent) event) - .getApplicationContext().getParent() == null) { - onContextClosedEvent(); - } else if (event instanceof ApplicationFailedEvent) { onApplicationFailedEvent(); } @@ -207,12 +211,6 @@ public class LoggingApplicationListener implements GenericApplicationListener { } } - private void onContextClosedEvent() { - if (this.loggingSystem != null) { - this.loggingSystem.cleanUp(); - } - } - private void onApplicationFailedEvent() { if (this.loggingSystem != null) { this.loggingSystem.cleanUp(); @@ -367,4 +365,41 @@ public class LoggingApplicationListener implements GenericApplicationListener { this.parseArgs = parseArgs; } + @Override + public void initialize(ConfigurableApplicationContext applicationContext) { + registerCleanupBean(applicationContext); + } + + private void registerCleanupBean(ConfigurableApplicationContext applicationContext) { + BeanFactory beanFactory = applicationContext.getBeanFactory(); + BeanDefinition beanDefinition = BeanDefinitionBuilder + .genericBeanDefinition(LoggingSystemCleanup.class) + .addConstructorArgValue(this.loggingSystem) + .addConstructorArgValue(applicationContext).getBeanDefinition(); + ((BeanDefinitionRegistry) beanFactory).registerBeanDefinition( + LoggingSystemCleanup.class.getName(), beanDefinition); + } + + private static final class LoggingSystemCleanup implements DisposableBean { + + private final LoggingSystem loggingSystem; + + private final ApplicationContext applicationContext; + + private LoggingSystemCleanup(LoggingSystem loggingSystem, + ApplicationContext applicationContext) { + this.loggingSystem = loggingSystem; + this.applicationContext = applicationContext; + } + + @Override + public void destroy() throws Exception { + if (this.applicationContext.getParent() == null + && this.loggingSystem != null) { + this.loggingSystem.cleanUp(); + } + } + + } + } diff --git a/spring-boot-project/spring-boot/src/main/resources/META-INF/spring.factories b/spring-boot-project/spring-boot/src/main/resources/META-INF/spring.factories index ff98c96f03..29e66d688c 100644 --- a/spring-boot-project/spring-boot/src/main/resources/META-INF/spring.factories +++ b/spring-boot-project/spring-boot/src/main/resources/META-INF/spring.factories @@ -16,6 +16,7 @@ org.springframework.context.ApplicationContextInitializer=\ org.springframework.boot.context.ConfigurationWarningsApplicationContextInitializer,\ org.springframework.boot.context.ContextIdApplicationContextInitializer,\ org.springframework.boot.context.config.DelegatingApplicationContextInitializer,\ +org.springframework.boot.context.logging.LoggingSystemLifecycle,\ org.springframework.boot.web.context.ServerPortInfoApplicationContextInitializer # Application Listeners @@ -27,7 +28,7 @@ org.springframework.boot.context.config.AnsiOutputApplicationListener,\ org.springframework.boot.context.config.ConfigFileApplicationListener,\ org.springframework.boot.context.config.DelegatingApplicationListener,\ org.springframework.boot.context.logging.ClasspathLoggingApplicationListener,\ -org.springframework.boot.context.logging.LoggingApplicationListener,\ +org.springframework.boot.context.logging.LoggingSystemLifecycle,\ org.springframework.boot.liquibase.LiquibaseServiceLocatorApplicationListener # Environment Post Processors diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/builder/SpringApplicationBuilderTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/builder/SpringApplicationBuilderTests.java index 69857218fc..0d241ac05a 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/builder/SpringApplicationBuilderTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/builder/SpringApplicationBuilderTests.java @@ -277,7 +277,7 @@ public class SpringApplicationBuilderTests { SpringApplicationBuilder application = new SpringApplicationBuilder( ExampleConfig.class).web(WebApplicationType.NONE); this.context = application.run(); - assertThat(application.application().getInitializers()).hasSize(4); + assertThat(application.application().getInitializers()).hasSize(5); } @Test @@ -286,7 +286,7 @@ public class SpringApplicationBuilderTests { ExampleConfig.class).child(ChildConfig.class) .web(WebApplicationType.NONE); this.context = application.run(); - assertThat(application.application().getInitializers()).hasSize(5); + assertThat(application.application().getInitializers()).hasSize(6); } @Test @@ -296,7 +296,7 @@ public class SpringApplicationBuilderTests { (ConfigurableApplicationContext applicationContext) -> { }); this.context = application.run(); - assertThat(application.application().getInitializers()).hasSize(5); + assertThat(application.application().getInitializers()).hasSize(6); } @Test diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/logging/LoggingApplicationListenerIntegrationTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/logging/LoggingSystemLifecycleIntegrationTests.java similarity index 93% rename from spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/logging/LoggingApplicationListenerIntegrationTests.java rename to spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/logging/LoggingSystemLifecycleIntegrationTests.java index 1990744c1f..9f08bab385 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/logging/LoggingApplicationListenerIntegrationTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/logging/LoggingSystemLifecycleIntegrationTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2017 the original author or authors. + * Copyright 2012-2018 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. @@ -33,11 +33,11 @@ import org.springframework.stereotype.Component; import static org.assertj.core.api.Assertions.assertThat; /** - * Integration tests for {@link LoggingApplicationListener}. + * Integration tests for {@link LoggingSystemLifecycle}. * * @author Stephane Nicoll */ -public class LoggingApplicationListenerIntegrationTests { +public class LoggingSystemLifecycleIntegrationTests { @Rule public OutputCapture outputCapture = new OutputCapture(); diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/logging/LoggingApplicationListenerTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/logging/LoggingSystemLifecycleTests.java similarity index 96% rename from spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/logging/LoggingApplicationListenerTests.java rename to spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/logging/LoggingSystemLifecycleTests.java index 714254c41f..ae6c2087ef 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/logging/LoggingApplicationListenerTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/logging/LoggingSystemLifecycleTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2017 the original author or authors. + * Copyright 2012-2018 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. @@ -56,7 +56,6 @@ import org.springframework.boot.testsupport.runner.classpath.ClassPathExclusions import org.springframework.boot.testsupport.runner.classpath.ModifiedClassPathRunner; import org.springframework.context.ApplicationEvent; import org.springframework.context.ApplicationListener; -import org.springframework.context.event.ContextClosedEvent; import org.springframework.context.event.SimpleApplicationEventMulticaster; import org.springframework.context.support.GenericApplicationContext; import org.springframework.core.env.ConfigurableEnvironment; @@ -70,7 +69,7 @@ import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.not; /** - * Tests for {@link LoggingApplicationListener} with Logback. + * Tests for {@link LoggingSystemLifecycle} with Logback. * * @author Dave Syer * @author Phillip Webb @@ -80,7 +79,7 @@ import static org.hamcrest.Matchers.not; */ @RunWith(ModifiedClassPathRunner.class) @ClassPathExclusions("log4j*.jar") -public class LoggingApplicationListenerTests { +public class LoggingSystemLifecycleTests { private static final String[] NO_ARGS = {}; @@ -93,7 +92,7 @@ public class LoggingApplicationListenerTests { @Rule public OutputCapture outputCapture = new OutputCapture(); - private final LoggingApplicationListener initializer = new LoggingApplicationListener(); + private final LoggingSystemLifecycle initializer = new LoggingSystemLifecycle(); private final Log logger = new SLF4JLogFactory().getInstance(getClass()); @@ -218,7 +217,7 @@ public class LoggingApplicationListenerTests { "logging.file=target/foo.log"); this.initializer.initialize(this.context.getEnvironment(), this.context.getClassLoader()); - Log logger = LogFactory.getLog(LoggingApplicationListenerTests.class); + Log logger = LogFactory.getLog(LoggingSystemLifecycleTests.class); logger.info("Hello world"); String output = this.outputCapture.toString().trim(); assertThat(output).startsWith("target/foo.log"); @@ -231,7 +230,7 @@ public class LoggingApplicationListenerTests { "logging.file=target/foo.log"); this.initializer.initialize(this.context.getEnvironment(), this.context.getClassLoader()); - Log logger = LogFactory.getLog(LoggingApplicationListenerTests.class); + Log logger = LogFactory.getLog(LoggingSystemLifecycleTests.class); logger.info("Hello world"); assertThat(new File("target/foo.log").exists()).isTrue(); } @@ -243,7 +242,7 @@ public class LoggingApplicationListenerTests { "logging.path=target/foo/"); this.initializer.initialize(this.context.getEnvironment(), this.context.getClassLoader()); - Log logger = LogFactory.getLog(LoggingApplicationListenerTests.class); + Log logger = LogFactory.getLog(LoggingSystemLifecycleTests.class); logger.info("Hello world"); String output = this.outputCapture.toString().trim(); assertThat(output).startsWith("target/foo/spring.log"); @@ -387,8 +386,10 @@ public class LoggingApplicationListenerTests { @Test public void bridgeHandlerLifecycle() { + this.initializer.initialize(this.context); + this.context.refresh(); assertThat(bridgeHandlerInstalled()).isTrue(); - multicastEvent(new ContextClosedEvent(this.context)); + this.context.close(); assertThat(bridgeHandlerInstalled()).isFalse(); } @@ -449,10 +450,12 @@ public class LoggingApplicationListenerTests { TestCleanupLoggingSystem.class.getName()); multicastEvent( new ApplicationStartingEvent(this.springApplication, new String[0])); + this.initializer.initialize(this.context); + this.context.refresh(); TestCleanupLoggingSystem loggingSystem = (TestCleanupLoggingSystem) ReflectionTestUtils .getField(this.initializer, "loggingSystem"); assertThat(loggingSystem.cleanedUp).isFalse(); - multicastEvent(new ContextClosedEvent(this.context)); + this.context.close(); assertThat(loggingSystem.cleanedUp).isTrue(); } @@ -462,16 +465,19 @@ public class LoggingApplicationListenerTests { TestCleanupLoggingSystem.class.getName()); multicastEvent( new ApplicationStartingEvent(this.springApplication, new String[0])); + this.initializer.initialize(this.context); + this.context.refresh(); TestCleanupLoggingSystem loggingSystem = (TestCleanupLoggingSystem) ReflectionTestUtils .getField(this.initializer, "loggingSystem"); assertThat(loggingSystem.cleanedUp).isFalse(); GenericApplicationContext childContext = new GenericApplicationContext(); childContext.setParent(this.context); - multicastEvent(new ContextClosedEvent(childContext)); - assertThat(loggingSystem.cleanedUp).isFalse(); - multicastEvent(new ContextClosedEvent(this.context)); - assertThat(loggingSystem.cleanedUp).isTrue(); + this.initializer.initialize(childContext); + childContext.refresh(); childContext.close(); + assertThat(loggingSystem.cleanedUp).isFalse(); + this.context.close(); + assertThat(loggingSystem.cleanedUp).isTrue(); } @Test @@ -613,8 +619,7 @@ public class LoggingApplicationListenerTests { } - public static class TestLoggingApplicationListener - extends LoggingApplicationListener { + public static class TestLoggingApplicationListener extends LoggingSystemLifecycle { private Thread shutdownHook;