From dbb24286ff2a9e2b159d33029f792cdf10397924 Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Wed, 31 May 2023 12:25:01 +0100 Subject: [PATCH] Separate stopping and destruction so web server can be restarted Previously, when a Servlet-based WebServer was stopped it would also stop the ServletContext. This led to problems as Tomcat and Undertow would then not allow a restart. Jetty would allow a restart but duplicate servlet registrations would then be attempted. This commit modifies the WebServer lifecycle to separate stopping and destruction for both servlet and reactive web servers. This allows a WebServer's stop() implementation to leave some components running so that they can be restarted. To completely shut down a WebServer destroy() must now be called. Both Tomcat and Jetty WebServers have been updated to stop their network connections when stop() is called but leave other components running. This works with both servlet and reactive web servers. Note that an Undertow-based Servlet web server does not support stop and restart. Once stopped, a Servlet Deployment cannot be restarted and it does not appear to be possible to separate the lifecycle of its network connections and a Servlet deployment. Reactor Netty and Undertow-based reactive web servers can now also be stopped and then restarted. Calling stop() stops the whole server but this does not cause a problem as there's no (application-exposed) ServletContext involved. There may be room to optimize this in the future if the need arises. Closes gh-34955 --- .../web/embedded/jetty/JettyWebServer.java | 16 +++++++- .../web/embedded/tomcat/GracefulShutdown.java | 14 +++---- .../web/embedded/tomcat/TomcatWebServer.java | 26 ++++++++----- .../embedded/undertow/UndertowWebServer.java | 6 +-- .../boot/web/server/WebServer.java | 10 ++++- .../ServletWebServerApplicationContext.java | 7 +++- .../JettyServletWebServerFactoryTests.java | 4 +- .../TomcatServletWebServerFactoryTests.java | 4 +- .../UndertowServletWebServerFactoryTests.java | 15 ++++++++ ...AbstractReactiveWebServerFactoryTests.java | 33 ++++++++++++++++- ...rvletWebServerApplicationContextTests.java | 28 +++++++++++++- .../AbstractServletWebServerFactoryTests.java | 37 +++++++++++++++++-- .../boot/loaderapp/LoaderTestApplication.java | 4 +- 13 files changed, 169 insertions(+), 35 deletions(-) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/jetty/JettyWebServer.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/jetty/JettyWebServer.java index 95ae2e3c2b..21f1052e6e 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/jetty/JettyWebServer.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/jetty/JettyWebServer.java @@ -238,7 +238,9 @@ public class JettyWebServer implements WebServer { this.gracefulShutdown.abort(); } try { - this.server.stop(); + for (Connector connector : this.server.getConnectors()) { + connector.stop(); + } } catch (InterruptedException ex) { Thread.currentThread().interrupt(); @@ -249,6 +251,18 @@ public class JettyWebServer implements WebServer { } } + @Override + public void destroy() { + synchronized (this.monitor) { + try { + this.server.stop(); + } + catch (Exception ex) { + throw new WebServerException("Unable to destroy embedded Jetty server", ex); + } + } + } + @Override public int getPort() { Connector[] connectors = this.server.getConnectors(); diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/GracefulShutdown.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/GracefulShutdown.java index c921cf5c94..3215a0de86 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/GracefulShutdown.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/GracefulShutdown.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2020 the original author or authors. + * Copyright 2012-2023 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. @@ -60,14 +60,14 @@ final class GracefulShutdown { try { for (Container host : this.tomcat.getEngine().findChildren()) { for (Container context : host.findChildren()) { - while (isActive(context)) { - if (this.aborted) { - logger.info("Graceful shutdown aborted with one or more requests still active"); - callback.shutdownComplete(GracefulShutdownResult.REQUESTS_ACTIVE); - return; - } + while (!this.aborted && isActive(context)) { Thread.sleep(50); } + if (this.aborted) { + logger.info("Graceful shutdown aborted with one or more requests still active"); + callback.shutdownComplete(GracefulShutdownResult.REQUESTS_ACTIVE); + return; + } } } diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/TomcatWebServer.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/TomcatWebServer.java index 7c05aa77f3..c2dfde03e9 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/TomcatWebServer.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/TomcatWebServer.java @@ -209,6 +209,7 @@ public class TomcatWebServer implements WebServer { if (this.started) { return; } + try { addPreviouslyRemovedConnectors(); Connector connector = this.tomcat.getConnector(); @@ -324,16 +325,10 @@ public class TomcatWebServer implements WebServer { boolean wasStarted = this.started; try { this.started = false; - try { - if (this.gracefulShutdown != null) { - this.gracefulShutdown.abort(); - } - stopTomcat(); - this.tomcat.destroy(); - } - catch (LifecycleException ex) { - // swallow and continue + if (this.gracefulShutdown != null) { + this.gracefulShutdown.abort(); } + removeServiceConnectors(); } catch (Exception ex) { throw new WebServerException("Unable to stop embedded Tomcat", ex); @@ -346,6 +341,19 @@ public class TomcatWebServer implements WebServer { } } + public void destroy() throws WebServerException { + try { + stopTomcat(); + this.tomcat.destroy(); + } + catch (LifecycleException ex) { + // Swallow and continue + } + catch (Exception ex) { + throw new WebServerException("Unable to destroy embedded Tomcat", ex); + } + } + private String getPortsDescription(boolean localPort) { StringBuilder ports = new StringBuilder(); for (Connector connector : this.tomcat.getService().findConnectors()) { diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/undertow/UndertowWebServer.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/undertow/UndertowWebServer.java index dd7f887bfb..563b2fc4b1 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/undertow/UndertowWebServer.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/undertow/UndertowWebServer.java @@ -132,13 +132,13 @@ public class UndertowWebServer implements WebServer { throw new WebServerException("Unable to start embedded Undertow", ex); } finally { - stopSilently(); + destroySilently(); } } } } - private void stopSilently() { + private void destroySilently() { try { if (this.undertow != null) { this.undertow.stop(); @@ -274,7 +274,7 @@ public class UndertowWebServer implements WebServer { } } catch (Exception ex) { - throw new WebServerException("Unable to stop undertow", ex); + throw new WebServerException("Unable to stop Undertow", ex); } } } diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/server/WebServer.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/server/WebServer.java index 6b70c02ca7..e5c02a86f8 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/server/WebServer.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/server/WebServer.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2020 the original author or authors. + * Copyright 2012-2023 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. @@ -61,4 +61,12 @@ public interface WebServer { callback.shutdownComplete(GracefulShutdownResult.IMMEDIATE); } + /** + * Destroys the web server such that it cannot be started again. + * @since 3.2.0 + */ + default void destroy() { + stop(); + } + } diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/servlet/context/ServletWebServerApplicationContext.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/servlet/context/ServletWebServerApplicationContext.java index 534237cc77..d6513792d2 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/servlet/context/ServletWebServerApplicationContext.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/servlet/context/ServletWebServerApplicationContext.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2022 the original author or authors. + * Copyright 2012-2023 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. @@ -149,6 +149,7 @@ public class ServletWebServerApplicationContext extends GenericWebApplicationCon WebServer webServer = this.webServer; if (webServer != null) { webServer.stop(); + webServer.destroy(); } throw ex; } @@ -171,6 +172,10 @@ public class ServletWebServerApplicationContext extends GenericWebApplicationCon AvailabilityChangeEvent.publish(this, ReadinessState.REFUSING_TRAFFIC); } super.doClose(); + WebServer webServer = this.webServer; + if (webServer != null) { + webServer.destroy(); + } } private void createWebServer() { diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/jetty/JettyServletWebServerFactoryTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/jetty/JettyServletWebServerFactoryTests.java index f037202ca4..9dc3ec21c5 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/jetty/JettyServletWebServerFactoryTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/jetty/JettyServletWebServerFactoryTests.java @@ -234,10 +234,10 @@ class JettyServletWebServerFactoryTests extends AbstractServletWebServerFactoryT } @Test - void stopCalledWithoutStart() { + void destroyCalledWithoutStart() { JettyServletWebServerFactory factory = getFactory(); this.webServer = factory.getWebServer(exampleServletRegistration()); - this.webServer.stop(); + this.webServer.destroy(); Server server = ((JettyWebServer) this.webServer).getServer(); assertThat(server.isStopped()).isTrue(); } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/tomcat/TomcatServletWebServerFactoryTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/tomcat/TomcatServletWebServerFactoryTests.java index 46e8eb59eb..ba8c07b6c0 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/tomcat/TomcatServletWebServerFactoryTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/tomcat/TomcatServletWebServerFactoryTests.java @@ -340,10 +340,10 @@ class TomcatServletWebServerFactoryTests extends AbstractServletWebServerFactory } @Test - void stopCalledWithoutStart() { + void destroyCalledWithoutStart() { TomcatServletWebServerFactory factory = getFactory(); this.webServer = factory.getWebServer(exampleServletRegistration()); - this.webServer.stop(); + this.webServer.destroy(); Tomcat tomcat = ((TomcatWebServer) this.webServer).getTomcat(); assertThat(tomcat.getServer().getState()).isSameAs(LifecycleState.DESTROYED); } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/undertow/UndertowServletWebServerFactoryTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/undertow/UndertowServletWebServerFactoryTests.java index dd42e2bf99..f0c5f7d4bd 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/undertow/UndertowServletWebServerFactoryTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/undertow/UndertowServletWebServerFactoryTests.java @@ -40,6 +40,7 @@ import org.apache.hc.core5.http.HttpResponse; import org.apache.jasper.servlet.JspServlet; import org.awaitility.Awaitility; import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.mockito.InOrder; @@ -211,6 +212,20 @@ class UndertowServletWebServerFactoryTests extends AbstractServletWebServerFacto this.webServer.stop(); } + @Test + @Override + @Disabled("Restart after stop is not supported with Undertow") + protected void restartAfterStop() { + + } + + @Test + @Override + @Disabled("Undertow's architecture prevents separating stop and destroy") + protected void servletContextListenerContextDestroyedIsNotCalledWhenContainerIsStopped() { + + } + private void testAccessLog(String prefix, String suffix, String expectedFile) throws IOException, URISyntaxException { UndertowServletWebServerFactory factory = getFactory(); diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/reactive/server/AbstractReactiveWebServerFactoryTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/reactive/server/AbstractReactiveWebServerFactoryTests.java index 2985bf9d34..2fb5865808 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/reactive/server/AbstractReactiveWebServerFactoryTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/reactive/server/AbstractReactiveWebServerFactoryTests.java @@ -77,6 +77,7 @@ import org.springframework.web.reactive.function.client.WebClient; import org.springframework.web.reactive.function.client.WebClientRequestException; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatException; import static org.assertj.core.api.Assertions.assertThatIllegalStateException; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -95,6 +96,12 @@ public abstract class AbstractReactiveWebServerFactoryTests { if (this.webServer != null) { try { this.webServer.stop(); + try { + this.webServer.destroy(); + } + catch (Exception ex) { + // Ignore + } } catch (Exception ex) { // Ignore @@ -124,13 +131,37 @@ public abstract class AbstractReactiveWebServerFactoryTests { assertThat(this.webServer.getPort()).isEqualTo(specificPort); } + @Test + protected void restartAfterStop() throws Exception { + AbstractReactiveWebServerFactory factory = getFactory(); + this.webServer = factory.getWebServer(new EchoHandler()); + this.webServer.start(); + int port = this.webServer.getPort(); + assertThat(getResponse(port, "/test")).isEqualTo("Hello World"); + this.webServer.stop(); + assertThatException().isThrownBy(() -> getResponse(port, "/test")); + this.webServer.start(); + assertThat(getResponse(this.webServer.getPort(), "/test")).isEqualTo("Hello World"); + } + + private String getResponse(int port, String uri) { + WebClient webClient = getWebClient(port).build(); + Mono result = webClient.post() + .uri(uri) + .contentType(MediaType.TEXT_PLAIN) + .body(BodyInserters.fromValue("Hello World")) + .retrieve() + .bodyToMono(String.class); + return result.block(Duration.ofSeconds(30)); + } + @Test void portIsMinusOneWhenConnectionIsClosed() { AbstractReactiveWebServerFactory factory = getFactory(); this.webServer = factory.getWebServer(new EchoHandler()); this.webServer.start(); assertThat(this.webServer.getPort()).isGreaterThan(0); - this.webServer.stop(); + this.webServer.destroy(); assertThat(this.webServer.getPort()).isEqualTo(-1); } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/context/ServletWebServerApplicationContextTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/context/ServletWebServerApplicationContextTests.java index 4bde935627..059b6224c2 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/context/ServletWebServerApplicationContextTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/context/ServletWebServerApplicationContextTests.java @@ -84,6 +84,7 @@ import static org.mockito.BDDMockito.then; import static org.mockito.Mockito.atMost; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.withSettings; /** @@ -156,12 +157,35 @@ class ServletWebServerApplicationContextTests { } @Test - void stopOnClose() { + void stopOnStop() { + addWebServerFactoryBean(); + this.context.refresh(); + MockServletWebServerFactory factory = getWebServerFactory(); + then(factory.getWebServer()).should().start(); + this.context.stop(); + then(factory.getWebServer()).should().stop(); + } + + @Test + void startOnStartAfterStop() { + addWebServerFactoryBean(); + this.context.refresh(); + MockServletWebServerFactory factory = getWebServerFactory(); + then(factory.getWebServer()).should().start(); + this.context.stop(); + then(factory.getWebServer()).should().stop(); + this.context.start(); + then(factory.getWebServer()).should(times(2)).start(); + } + + @Test + void stopAndDestroyOnClose() { addWebServerFactoryBean(); this.context.refresh(); MockServletWebServerFactory factory = getWebServerFactory(); this.context.close(); - then(factory.getWebServer()).should().stop(); + then(factory.getWebServer()).should(times(2)).stop(); + then(factory.getWebServer()).should().destroy(); } @Test diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/server/AbstractServletWebServerFactoryTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/server/AbstractServletWebServerFactoryTests.java index 12a9aea44c..c6c53781da 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/server/AbstractServletWebServerFactoryTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/server/AbstractServletWebServerFactoryTests.java @@ -167,6 +167,7 @@ import static org.mockito.BDDMockito.then; import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; /** * Base for testing classes that extends {@link AbstractServletWebServerFactory}. @@ -197,6 +198,12 @@ public abstract class AbstractServletWebServerFactoryTests { if (this.webServer != null) { try { this.webServer.stop(); + try { + this.webServer.destroy(); + } + catch (Exception ex) { + // Ignore + } } catch (Exception ex) { // Ignore @@ -233,6 +240,19 @@ public abstract class AbstractServletWebServerFactoryTests { this.webServer.stop(); } + @Test + protected void restartAfterStop() throws IOException, URISyntaxException { + AbstractServletWebServerFactory factory = getFactory(); + this.webServer = factory.getWebServer(exampleServletRegistration()); + this.webServer.start(); + assertThat(getResponse(getLocalUrl("/hello"))).isEqualTo("Hello World"); + int port = this.webServer.getPort(); + this.webServer.stop(); + assertThatIOException().isThrownBy(() -> getResponse(getLocalUrl(port, "/hello"))); + this.webServer.start(); + assertThat(getResponse(getLocalUrl("/hello"))).isEqualTo("Hello World"); + } + @Test void emptyServerWhenPortIsMinusOne() { AbstractServletWebServerFactory factory = getFactory(); @@ -295,7 +315,7 @@ public abstract class AbstractServletWebServerFactoryTests { this.webServer = factory.getWebServer(); this.webServer.start(); assertThat(this.webServer.getPort()).isGreaterThan(0); - this.webServer.stop(); + this.webServer.destroy(); assertThat(this.webServer.getPort()).isEqualTo(-1); } @@ -814,7 +834,7 @@ public abstract class AbstractServletWebServerFactoryTests { this.webServer.start(); String s1 = getResponse(getLocalUrl("/session")); String s2 = getResponse(getLocalUrl("/session")); - this.webServer.stop(); + this.webServer.destroy(); this.webServer = factory.getWebServer(sessionServletRegistration()); this.webServer.start(); String s3 = getResponse(getLocalUrl("/session")); @@ -833,7 +853,7 @@ public abstract class AbstractServletWebServerFactoryTests { this.webServer = factory.getWebServer(sessionServletRegistration()); this.webServer.start(); getResponse(getLocalUrl("/session")); - this.webServer.stop(); + this.webServer.destroy(); File[] dirContents = sessionStoreDir.listFiles((dir, name) -> !(".".equals(name) || "..".equals(name))); assertThat(dirContents).isNotEmpty(); } @@ -1158,11 +1178,20 @@ public abstract class AbstractServletWebServerFactoryTests { } @Test - void servletContextListenerContextDestroyedIsCalledWhenContainerIsStopped() throws Exception { + protected void servletContextListenerContextDestroyedIsNotCalledWhenContainerIsStopped() throws Exception { ServletContextListener listener = mock(ServletContextListener.class); this.webServer = getFactory().getWebServer((servletContext) -> servletContext.addListener(listener)); this.webServer.start(); this.webServer.stop(); + then(listener).should(times(0)).contextDestroyed(any(ServletContextEvent.class)); + } + + @Test + void servletContextListenerContextDestroyedIsCalledWhenContainerIsDestroyed() throws Exception { + ServletContextListener listener = mock(ServletContextListener.class); + this.webServer = getFactory().getWebServer((servletContext) -> servletContext.addListener(listener)); + this.webServer.start(); + this.webServer.destroy(); then(listener).should().contextDestroyed(any(ServletContextEvent.class)); } diff --git a/spring-boot-tests/spring-boot-integration-tests/spring-boot-loader-tests/spring-boot-loader-tests-app/src/main/java/org/springframework/boot/loaderapp/LoaderTestApplication.java b/spring-boot-tests/spring-boot-integration-tests/spring-boot-loader-tests/spring-boot-loader-tests-app/src/main/java/org/springframework/boot/loaderapp/LoaderTestApplication.java index 81b7c41cbd..0c9d429350 100644 --- a/spring-boot-tests/spring-boot-integration-tests/spring-boot-loader-tests/spring-boot-loader-tests-app/src/main/java/org/springframework/boot/loaderapp/LoaderTestApplication.java +++ b/spring-boot-tests/spring-boot-integration-tests/spring-boot-loader-tests/spring-boot-loader-tests-app/src/main/java/org/springframework/boot/loaderapp/LoaderTestApplication.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2021 the original author or authors. + * Copyright 2012-2023 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. @@ -53,7 +53,7 @@ public class LoaderTestApplication { } public static void main(String[] args) { - SpringApplication.run(LoaderTestApplication.class, args).stop(); + SpringApplication.run(LoaderTestApplication.class, args).close(); } }