From 757aa647cfa44196c93f5730d0f7b33032b5981f Mon Sep 17 00:00:00 2001 From: "Michael K. Werle" Date: Tue, 7 Feb 2017 17:20:06 -0600 Subject: [PATCH 1/3] Ensure web containers are stopped after close Update `EmbeddedServletContainer` implementations to ensure that stop can be called even if start has not. This allows servers that are partially started during `initialize()` to still be shut down. This commit fixes a regression caused by commit 0af53b361f. See gh-8036 Fixes gh-8224 Closes gh-8227 --- .../embedded/jetty/JettyEmbeddedServletContainer.java | 3 --- .../tomcat/TomcatEmbeddedServletContainer.java | 3 --- .../JettyEmbeddedServletContainerFactoryTests.java | 10 ++++++++++ .../TomcatEmbeddedServletContainerFactoryTests.java | 10 ++++++++++ 4 files changed, 20 insertions(+), 6 deletions(-) diff --git a/spring-boot/src/main/java/org/springframework/boot/context/embedded/jetty/JettyEmbeddedServletContainer.java b/spring-boot/src/main/java/org/springframework/boot/context/embedded/jetty/JettyEmbeddedServletContainer.java index a52a789078..8941c3d614 100644 --- a/spring-boot/src/main/java/org/springframework/boot/context/embedded/jetty/JettyEmbeddedServletContainer.java +++ b/spring-boot/src/main/java/org/springframework/boot/context/embedded/jetty/JettyEmbeddedServletContainer.java @@ -205,9 +205,6 @@ public class JettyEmbeddedServletContainer implements EmbeddedServletContainer { @Override public void stop() { synchronized (this.monitor) { - if (!this.started) { - return; - } this.started = false; try { this.server.stop(); diff --git a/spring-boot/src/main/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainer.java b/spring-boot/src/main/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainer.java index 27ab3dc021..b97a66574d 100644 --- a/spring-boot/src/main/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainer.java +++ b/spring-boot/src/main/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainer.java @@ -279,9 +279,6 @@ public class TomcatEmbeddedServletContainer implements EmbeddedServletContainer @Override public void stop() throws EmbeddedServletContainerException { synchronized (this.monitor) { - if (!this.started) { - return; - } try { this.started = false; try { diff --git a/spring-boot/src/test/java/org/springframework/boot/context/embedded/jetty/JettyEmbeddedServletContainerFactoryTests.java b/spring-boot/src/test/java/org/springframework/boot/context/embedded/jetty/JettyEmbeddedServletContainerFactoryTests.java index cedeaae784..265b7e573b 100644 --- a/spring-boot/src/test/java/org/springframework/boot/context/embedded/jetty/JettyEmbeddedServletContainerFactoryTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/context/embedded/jetty/JettyEmbeddedServletContainerFactoryTests.java @@ -143,6 +143,16 @@ public class JettyEmbeddedServletContainerFactoryTests .isEmpty(); } + @Test + public void stopNoStart() throws Exception { + JettyEmbeddedServletContainerFactory factory = getFactory(); + this.container = factory + .getEmbeddedServletContainer(exampleServletRegistration()); + this.container.stop(); + Server server = ((JettyEmbeddedServletContainer) this.container).getServer(); + assertThat(server.isStopped()).isTrue(); + } + @Override protected void addConnector(final int port, AbstractEmbeddedServletContainerFactory factory) { diff --git a/spring-boot/src/test/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainerFactoryTests.java b/spring-boot/src/test/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainerFactoryTests.java index ac3690d905..cbb54278b7 100644 --- a/spring-boot/src/test/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainerFactoryTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainerFactoryTests.java @@ -352,6 +352,16 @@ public class TomcatEmbeddedServletContainerFactoryTests .doesNotContain("appears to have started a thread named [main]"); } + @Test + public void stopNoStart() throws Exception { + TomcatEmbeddedServletContainerFactory factory = getFactory(); + this.container = factory + .getEmbeddedServletContainer(exampleServletRegistration()); + this.container.stop(); + Tomcat tomcat = ((TomcatEmbeddedServletContainer) this.container).getTomcat(); + assertThat(tomcat.getServer().getState()).isSameAs(LifecycleState.DESTROYED); + } + @Override protected void addConnector(int port, AbstractEmbeddedServletContainerFactory factory) { From 7fda9c162e16580735bbf39dcf96c0a4874d459c Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Wed, 1 Mar 2017 22:02:25 -0800 Subject: [PATCH 2/3] Polish web containers stop contribution See gh-8227 --- ...tEmbeddedServletContainerFactoryTests.java | 20 +++++++++---------- ...yEmbeddedServletContainerFactoryTests.java | 2 +- ...tEmbeddedServletContainerFactoryTests.java | 2 +- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/spring-boot/src/test/java/org/springframework/boot/context/embedded/AbstractEmbeddedServletContainerFactoryTests.java b/spring-boot/src/test/java/org/springframework/boot/context/embedded/AbstractEmbeddedServletContainerFactoryTests.java index 11482e73a9..6f30f6dd10 100644 --- a/spring-boot/src/test/java/org/springframework/boot/context/embedded/AbstractEmbeddedServletContainerFactoryTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/context/embedded/AbstractEmbeddedServletContainerFactoryTests.java @@ -170,6 +170,16 @@ public abstract class AbstractEmbeddedServletContainerFactoryTests { assertThat(this.output.toString()).containsOnlyOnce("started on port"); } + @Test + public void stopCalledTwice() throws Exception { + AbstractEmbeddedServletContainerFactory factory = getFactory(); + this.container = factory + .getEmbeddedServletContainer(exampleServletRegistration()); + this.container.start(); + this.container.stop(); + this.container.stop(); + } + @Test public void emptyServerWhenPortIsMinusOne() throws Exception { AbstractEmbeddedServletContainerFactory factory = getFactory(); @@ -311,16 +321,6 @@ public abstract class AbstractEmbeddedServletContainerFactoryTests { getFactory().setContextPath("/"); } - @Test - public void doubleStop() throws Exception { - AbstractEmbeddedServletContainerFactory factory = getFactory(); - this.container = factory - .getEmbeddedServletContainer(exampleServletRegistration()); - this.container.start(); - this.container.stop(); - this.container.stop(); - } - @Test public void multipleConfigurations() throws Exception { AbstractEmbeddedServletContainerFactory factory = getFactory(); diff --git a/spring-boot/src/test/java/org/springframework/boot/context/embedded/jetty/JettyEmbeddedServletContainerFactoryTests.java b/spring-boot/src/test/java/org/springframework/boot/context/embedded/jetty/JettyEmbeddedServletContainerFactoryTests.java index 265b7e573b..def6a3f8bd 100644 --- a/spring-boot/src/test/java/org/springframework/boot/context/embedded/jetty/JettyEmbeddedServletContainerFactoryTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/context/embedded/jetty/JettyEmbeddedServletContainerFactoryTests.java @@ -144,7 +144,7 @@ public class JettyEmbeddedServletContainerFactoryTests } @Test - public void stopNoStart() throws Exception { + public void stopCalledWithoutStart() throws Exception { JettyEmbeddedServletContainerFactory factory = getFactory(); this.container = factory .getEmbeddedServletContainer(exampleServletRegistration()); diff --git a/spring-boot/src/test/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainerFactoryTests.java b/spring-boot/src/test/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainerFactoryTests.java index cbb54278b7..1fa3534209 100644 --- a/spring-boot/src/test/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainerFactoryTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainerFactoryTests.java @@ -353,7 +353,7 @@ public class TomcatEmbeddedServletContainerFactoryTests } @Test - public void stopNoStart() throws Exception { + public void stopCalledWithoutStart() throws Exception { TomcatEmbeddedServletContainerFactory factory = getFactory(); this.container = factory .getEmbeddedServletContainer(exampleServletRegistration()); From 5aafbc2a3b1d633f8aa436025943146b10fae5ff Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Wed, 1 Mar 2017 22:36:14 -0800 Subject: [PATCH 3/3] Refine engine counter logic Update counter logic to prevent negative values. Since the stop method can now be called more than once, it was possible for the counter to move into negative values. See gh-8227 --- .../TomcatEmbeddedServletContainer.java | 51 +++++++++++-------- 1 file changed, 30 insertions(+), 21 deletions(-) diff --git a/spring-boot/src/main/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainer.java b/spring-boot/src/main/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainer.java index b97a66574d..b9a59b4fc2 100644 --- a/spring-boot/src/main/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainer.java +++ b/spring-boot/src/main/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainer.java @@ -90,28 +90,34 @@ public class TomcatEmbeddedServletContainer implements EmbeddedServletContainer synchronized (this.monitor) { try { addInstanceIdToEngineName(); - - // Remove service connectors to that protocol binding doesn't happen yet - removeServiceConnectors(); - - // Start the server to trigger initialization listeners - this.tomcat.start(); - - // We can re-throw failure exception directly in the main thread - rethrowDeferredStartupExceptions(); - - Context context = findContext(); try { - ContextBindings.bindClassLoader(context, getNamingToken(context), - getClass().getClassLoader()); - } - catch (NamingException ex) { - // Naming is not enabled. Continue - } + // Remove service connectors to that protocol binding doesn't happen + // yet + removeServiceConnectors(); - // Unlike Jetty, all Tomcat threads are daemon threads. We create a - // blocking non-daemon to stop immediate shutdown - startDaemonAwaitThread(); + // Start the server to trigger initialization listeners + this.tomcat.start(); + + // We can re-throw failure exception directly in the main thread + rethrowDeferredStartupExceptions(); + + Context context = findContext(); + try { + ContextBindings.bindClassLoader(context, getNamingToken(context), + getClass().getClassLoader()); + } + catch (NamingException ex) { + // Naming is not enabled. Continue + } + + // Unlike Jetty, all Tomcat threads are daemon threads. We create a + // blocking non-daemon to stop immediate shutdown + startDaemonAwaitThread(); + } + catch (Exception ex) { + containerCounter.decrementAndGet(); + throw ex; + } } catch (Exception ex) { throw new EmbeddedServletContainerException( @@ -279,6 +285,7 @@ public class TomcatEmbeddedServletContainer implements EmbeddedServletContainer @Override public void stop() throws EmbeddedServletContainerException { synchronized (this.monitor) { + boolean wasStarted = this.started; try { this.started = false; try { @@ -294,7 +301,9 @@ public class TomcatEmbeddedServletContainer implements EmbeddedServletContainer "Unable to stop embedded Tomcat", ex); } finally { - containerCounter.decrementAndGet(); + if (wasStarted) { + containerCounter.decrementAndGet(); + } } } }