From 0069e41c29382a788a3515937a6a14a9b71c9107 Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Fri, 9 Oct 2015 12:24:43 +0100 Subject: [PATCH] Prevent closed context from being refreshed again by an HTTP request Prior to this commit, EmbeddedWebApplicationContext would perform its close processing and then stop the embedded container. This could lead to the closed context's dispatcher servlet handling an HTTP request and refreshing the context again. This opened up two possibilities that we need to avoid: 1. Another HTTP request could be received by the dispatcher servlet while the context is still being refreshed. This could lead to the context being used before its refreshed. I believe this could be the cause of the current modification exception described in gh-3239 and SPR-13123. 2. It can lead to a race during shutdown as the shutdown hook's attempt to close the context races with the refresh initiated by the HTTP request. This is possible as the shutdown hook bypasses the sychronization on startupShutdownMonitor that would normally prevent refresh and close from occurring in parallel. This race can lead to a deadlock as described in gh-4130 Closes gh-4130 --- .../EmbeddedWebApplicationContext.java | 2 +- .../EmbeddedWebApplicationContextTests.java | 41 +++++++++++++++++++ .../MockEmbeddedServletContainerFactory.java | 7 ++++ 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/spring-boot/src/main/java/org/springframework/boot/context/embedded/EmbeddedWebApplicationContext.java b/spring-boot/src/main/java/org/springframework/boot/context/embedded/EmbeddedWebApplicationContext.java index 02568a0140d..836d6b14f2e 100644 --- a/spring-boot/src/main/java/org/springframework/boot/context/embedded/EmbeddedWebApplicationContext.java +++ b/spring-boot/src/main/java/org/springframework/boot/context/embedded/EmbeddedWebApplicationContext.java @@ -146,8 +146,8 @@ public class EmbeddedWebApplicationContext extends GenericWebApplicationContext @Override protected void doClose() { - super.doClose(); stopAndReleaseEmbeddedServletContainer(); + super.doClose(); } private synchronized void createEmbeddedServletContainer() { diff --git a/spring-boot/src/test/java/org/springframework/boot/context/embedded/EmbeddedWebApplicationContextTests.java b/spring-boot/src/test/java/org/springframework/boot/context/embedded/EmbeddedWebApplicationContextTests.java index 9f79be5b768..037931796be 100644 --- a/spring-boot/src/test/java/org/springframework/boot/context/embedded/EmbeddedWebApplicationContextTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/context/embedded/EmbeddedWebApplicationContextTests.java @@ -36,11 +36,14 @@ import org.junit.Test; import org.junit.rules.ExpectedException; import org.mockito.InOrder; import org.springframework.beans.MutablePropertyValues; +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.config.ConstructorArgumentValues; import org.springframework.beans.factory.config.Scope; +import org.springframework.beans.factory.support.BeanDefinitionBuilder; import org.springframework.beans.factory.support.RootBeanDefinition; +import org.springframework.boot.context.embedded.MockEmbeddedServletContainerFactory.MockEmbeddedServletContainer; import org.springframework.context.ApplicationContextException; import org.springframework.context.ApplicationListener; import org.springframework.context.support.AbstractApplicationContext; @@ -54,6 +57,7 @@ import org.springframework.web.filter.GenericFilterBean; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.nullValue; import static org.hamcrest.Matchers.sameInstance; import static org.junit.Assert.assertEquals; @@ -429,6 +433,22 @@ public class EmbeddedWebApplicationContextTests { sameInstance(scope)); } + @Test + public void containerIsStoppedBeforeContextIsClosed() { + addEmbeddedServletContainerFactoryBean(); + this.context.registerBeanDefinition("shutdownOrderingValidator", + BeanDefinitionBuilder.rootBeanDefinition(ShutdownOrderingValidator.class) + .addConstructorArgReference("embeddedServletContainerFactory") + .getBeanDefinition()); + this.context.refresh(); + ShutdownOrderingValidator validator = this.context + .getBean(ShutdownOrderingValidator.class); + this.context.close(); + assertThat(validator.destroyed, is(true)); + assertThat(validator.containerStoppedFirst, is(true)); + + } + private void addEmbeddedServletContainerFactoryBean() { this.context.registerBeanDefinition("embeddedServletContainerFactory", new RootBeanDefinition(MockEmbeddedServletContainerFactory.class)); @@ -478,4 +498,25 @@ public class EmbeddedWebApplicationContextTests { } + protected static class ShutdownOrderingValidator implements DisposableBean { + + private final MockEmbeddedServletContainer servletContainer; + + private boolean destroyed = false; + + private boolean containerStoppedFirst = false; + + ShutdownOrderingValidator( + MockEmbeddedServletContainerFactory servletContainerFactory) { + this.servletContainer = servletContainerFactory.getContainer(); + } + + @Override + public void destroy() { + this.destroyed = true; + this.containerStoppedFirst = this.servletContainer.isStopped(); + } + + } + } diff --git a/spring-boot/src/test/java/org/springframework/boot/context/embedded/MockEmbeddedServletContainerFactory.java b/spring-boot/src/test/java/org/springframework/boot/context/embedded/MockEmbeddedServletContainerFactory.java index 6debacdb9ff..dcbdcdb67b7 100644 --- a/spring-boot/src/test/java/org/springframework/boot/context/embedded/MockEmbeddedServletContainerFactory.java +++ b/spring-boot/src/test/java/org/springframework/boot/context/embedded/MockEmbeddedServletContainerFactory.java @@ -90,6 +90,8 @@ public class MockEmbeddedServletContainerFactory private final int port; + private boolean stopped = false; + public MockEmbeddedServletContainer(ServletContextInitializer[] initializers, int port) { this.initializers = initializers; @@ -188,6 +190,11 @@ public class MockEmbeddedServletContainerFactory public void stop() { this.servletContext = null; this.registeredServlets.clear(); + this.stopped = true; + } + + public boolean isStopped() { + return this.stopped; } public Servlet[] getServlets() {