From ffa70f7c7e1a439a477474d712b12a5ef6002191 Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Thu, 18 Jul 2013 11:29:00 +0100 Subject: [PATCH] [bs-239] Race condition in embedded container contexts I'll mark this as fixing the bug in tracker, but it might benefit from some re-working at some point. The basic idea is to use our old friend ContextRefreshedEvent to start the server listening for connections. Delaying and making public the start() method from the EmbeddedServletContainer doesn't help because you need the server to start in order to get a ServletContext for Spring. [Fixes #53538787] --- ...stractEmbeddedServletContainerFactory.java | 15 +++++++- .../jetty/JettyEmbeddedServletContainer.java | 8 ++++ .../JettyEmbeddedServletContainerFactory.java | 23 +++++++++++- .../TomcatEmbeddedServletContainer.java | 2 + ...TomcatEmbeddedServletContainerFactory.java | 37 ++++++++++++++++--- ...tEmbeddedServletContainerFactoryTests.java | 33 ++++++++++++----- spring-starters/spring-starter-parent/pom.xml | 7 ++++ 7 files changed, 107 insertions(+), 18 deletions(-) diff --git a/spring-bootstrap/src/main/java/org/springframework/bootstrap/context/embedded/AbstractEmbeddedServletContainerFactory.java b/spring-bootstrap/src/main/java/org/springframework/bootstrap/context/embedded/AbstractEmbeddedServletContainerFactory.java index 434e7a2834..0d6750a263 100644 --- a/spring-bootstrap/src/main/java/org/springframework/bootstrap/context/embedded/AbstractEmbeddedServletContainerFactory.java +++ b/spring-bootstrap/src/main/java/org/springframework/bootstrap/context/embedded/AbstractEmbeddedServletContainerFactory.java @@ -31,6 +31,8 @@ import java.util.Set; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.context.ApplicationListener; +import org.springframework.context.event.ContextRefreshedEvent; import org.springframework.util.Assert; /** @@ -40,12 +42,13 @@ import org.springframework.util.Assert; * @author Dave Syer */ public abstract class AbstractEmbeddedServletContainerFactory implements - ConfigurableEmbeddedServletContainerFactory { + ConfigurableEmbeddedServletContainerFactory, + ApplicationListener { private static final String[] COMMON_DOC_ROOTS = { "src/main/webapp", "public", "static" }; - private final Log logger = LogFactory.getLog(getClass()); + protected final Log logger = LogFactory.getLog(getClass()); private String contextPath = ""; @@ -94,6 +97,14 @@ public abstract class AbstractEmbeddedServletContainerFactory implements this.port = port; } + /** + * Subclasses should use this event to tell the server to start listening for + * connections. + */ + @Override + public void onApplicationEvent(ContextRefreshedEvent event) { + } + /** * Sets the context path for the embedded servlet container. The context should start * with a "/" character but not end with a "/" character. The default context path can diff --git a/spring-bootstrap/src/main/java/org/springframework/bootstrap/context/embedded/jetty/JettyEmbeddedServletContainer.java b/spring-bootstrap/src/main/java/org/springframework/bootstrap/context/embedded/jetty/JettyEmbeddedServletContainer.java index 9d2c4c5720..34a122c9f6 100644 --- a/spring-bootstrap/src/main/java/org/springframework/bootstrap/context/embedded/jetty/JettyEmbeddedServletContainer.java +++ b/spring-bootstrap/src/main/java/org/springframework/bootstrap/context/embedded/jetty/JettyEmbeddedServletContainer.java @@ -16,6 +16,7 @@ package org.springframework.bootstrap.context.embedded.jetty; +import org.eclipse.jetty.server.Connector; import org.eclipse.jetty.server.Server; import org.springframework.bootstrap.context.embedded.EmbeddedServletContainer; import org.springframework.bootstrap.context.embedded.EmbeddedServletContainerException; @@ -46,6 +47,13 @@ public class JettyEmbeddedServletContainer implements EmbeddedServletContainer { private synchronized void start() { try { this.server.start(); + // Start the server so the ServletContext is available, but stop the + // connectors to prevent requests from being handled before the Spring context + // is ready: + Connector[] connectors = this.server.getConnectors(); + for (Connector connector : connectors) { + connector.stop(); + } } catch (Exception ex) { throw new EmbeddedServletContainerException( diff --git a/spring-bootstrap/src/main/java/org/springframework/bootstrap/context/embedded/jetty/JettyEmbeddedServletContainerFactory.java b/spring-bootstrap/src/main/java/org/springframework/bootstrap/context/embedded/jetty/JettyEmbeddedServletContainerFactory.java index 512b2ba766..4b05f9dd40 100644 --- a/spring-bootstrap/src/main/java/org/springframework/bootstrap/context/embedded/jetty/JettyEmbeddedServletContainerFactory.java +++ b/spring-bootstrap/src/main/java/org/springframework/bootstrap/context/embedded/jetty/JettyEmbeddedServletContainerFactory.java @@ -23,6 +23,7 @@ import java.util.Arrays; import java.util.Collection; import java.util.List; +import org.eclipse.jetty.server.Connector; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.handler.ErrorHandler; import org.eclipse.jetty.servlet.ErrorPageErrorHandler; @@ -34,10 +35,13 @@ import org.eclipse.jetty.webapp.Configuration; import org.eclipse.jetty.webapp.WebAppContext; import org.springframework.bootstrap.context.embedded.AbstractEmbeddedServletContainerFactory; import org.springframework.bootstrap.context.embedded.EmbeddedServletContainer; +import org.springframework.bootstrap.context.embedded.EmbeddedServletContainerException; import org.springframework.bootstrap.context.embedded.EmbeddedServletContainerFactory; import org.springframework.bootstrap.context.embedded.ErrorPage; import org.springframework.bootstrap.context.embedded.ServletContextInitializer; +import org.springframework.context.ApplicationListener; import org.springframework.context.ResourceLoaderAware; +import org.springframework.context.event.ContextRefreshedEvent; import org.springframework.core.io.ResourceLoader; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; @@ -59,7 +63,8 @@ import org.springframework.util.StringUtils; * @see JettyEmbeddedServletContainer */ public class JettyEmbeddedServletContainerFactory extends - AbstractEmbeddedServletContainerFactory implements ResourceLoaderAware { + AbstractEmbeddedServletContainerFactory implements ResourceLoaderAware, + ApplicationListener { private List configurations = new ArrayList(); @@ -93,6 +98,22 @@ public class JettyEmbeddedServletContainerFactory extends super(contextPath, port); } + @Override + public void onApplicationEvent(ContextRefreshedEvent event) { + if (this.context != null && this.context.getServer() != null) { + try { + Connector[] connectors = this.context.getServer().getConnectors(); + for (Connector connector : connectors) { + connector.start(); + } + } + catch (Exception ex) { + throw new EmbeddedServletContainerException( + "Unable to start embedded Jetty servlet container", ex); + } + } + } + @Override public EmbeddedServletContainer getEmbeddedServletContainer( ServletContextInitializer... initializers) { diff --git a/spring-bootstrap/src/main/java/org/springframework/bootstrap/context/embedded/tomcat/TomcatEmbeddedServletContainer.java b/spring-bootstrap/src/main/java/org/springframework/bootstrap/context/embedded/tomcat/TomcatEmbeddedServletContainer.java index 3d1d6d54f2..44e4b3a019 100644 --- a/spring-bootstrap/src/main/java/org/springframework/bootstrap/context/embedded/tomcat/TomcatEmbeddedServletContainer.java +++ b/spring-bootstrap/src/main/java/org/springframework/bootstrap/context/embedded/tomcat/TomcatEmbeddedServletContainer.java @@ -28,6 +28,8 @@ import org.springframework.util.Assert; * {@link TomcatEmbeddedServletContainerFactory} and not directly. * * @author Phillip Webb + * @author Dave Syer + * * @see TomcatEmbeddedServletContainerFactory */ public class TomcatEmbeddedServletContainer implements EmbeddedServletContainer { diff --git a/spring-bootstrap/src/main/java/org/springframework/bootstrap/context/embedded/tomcat/TomcatEmbeddedServletContainerFactory.java b/spring-bootstrap/src/main/java/org/springframework/bootstrap/context/embedded/tomcat/TomcatEmbeddedServletContainerFactory.java index 66983895be..dbe726dadc 100644 --- a/spring-bootstrap/src/main/java/org/springframework/bootstrap/context/embedded/tomcat/TomcatEmbeddedServletContainerFactory.java +++ b/spring-bootstrap/src/main/java/org/springframework/bootstrap/context/embedded/tomcat/TomcatEmbeddedServletContainerFactory.java @@ -45,6 +45,7 @@ import org.springframework.bootstrap.context.embedded.EmbeddedServletContainerFa import org.springframework.bootstrap.context.embedded.ErrorPage; import org.springframework.bootstrap.context.embedded.ServletContextInitializer; import org.springframework.context.ResourceLoaderAware; +import org.springframework.context.event.ContextRefreshedEvent; import org.springframework.core.io.ResourceLoader; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; @@ -60,6 +61,7 @@ import org.springframework.util.ClassUtils; * * @author Phillip Webb * @author Dave Syer + * * @see #setPort(int) * @see #setContextLifecycleListeners(Collection) * @see TomcatEmbeddedServletContainer @@ -105,6 +107,19 @@ public class TomcatEmbeddedServletContainerFactory extends super(contextPath, port); } + @Override + public void onApplicationEvent(ContextRefreshedEvent event) { + Connector connector = this.connector; + if (connector != null) { + try { + connector.getProtocolHandler().resume(); + } + catch (Exception e) { + this.logger.error("Cannot start connector: ", e); + } + } + } + @Override public EmbeddedServletContainer getEmbeddedServletContainer( ServletContextInitializer... initializers) { @@ -114,18 +129,28 @@ public class TomcatEmbeddedServletContainerFactory extends File baseDir = (this.baseDirectory != null ? this.baseDirectory : createTempDir("tomcat")); this.tomcat.setBaseDir(baseDir.getAbsolutePath()); - if (this.connector != null) { - this.connector.setPort(getPort()); - this.tomcat.getService().addConnector(this.connector); - this.tomcat.setConnector(this.connector); + Connector connector = this.connector; + if (connector != null) { + connector.setPort(getPort()); + this.tomcat.getService().addConnector(connector); + this.tomcat.setConnector(connector); } else { - Connector connector = new Connector( - "org.apache.coyote.http11.Http11NioProtocol"); + connector = new Connector("org.apache.coyote.http11.Http11NioProtocol"); customizeConnector(connector); this.tomcat.getService().addConnector(connector); this.tomcat.setConnector(connector); } + this.connector = connector; + try { + // Allow the server to start so the ServletContext is available, but stop the + // connector to prevent requests from being handled before the Spring context + // is ready: + connector.getProtocolHandler().pause(); + } + catch (Exception e) { + this.logger.error("Cannot pause connector: ", e); + } this.tomcat.getHost().setAutoDeploy(false); this.tomcat.getEngine().setBackgroundProcessorDelay(-1); diff --git a/spring-bootstrap/src/test/java/org/springframework/bootstrap/context/embedded/AbstractEmbeddedServletContainerFactoryTests.java b/spring-bootstrap/src/test/java/org/springframework/bootstrap/context/embedded/AbstractEmbeddedServletContainerFactoryTests.java index 281bd22846..c7d245de5f 100644 --- a/spring-bootstrap/src/test/java/org/springframework/bootstrap/context/embedded/AbstractEmbeddedServletContainerFactoryTests.java +++ b/spring-bootstrap/src/test/java/org/springframework/bootstrap/context/embedded/AbstractEmbeddedServletContainerFactoryTests.java @@ -84,17 +84,18 @@ public abstract class AbstractEmbeddedServletContainerFactoryTests { ConfigurableEmbeddedServletContainerFactory factory = getFactory(); this.container = factory .getEmbeddedServletContainer(exampleServletRegistration()); - assertThat(getResponse("http://localhost:8080/hello"), equalTo("Hello World")); + assertThat(getResponse(factory, "http://localhost:8080/hello"), + equalTo("Hello World")); } @Test - public void emptyServer() throws Exception { + public void emptyServerWhenPortIsZero() throws Exception { ConfigurableEmbeddedServletContainerFactory factory = getFactory(); factory.setPort(0); this.container = factory .getEmbeddedServletContainer(exampleServletRegistration()); this.thrown.expect(SocketException.class); - getResponse("http://localhost:8080/hello"); + getResponse(factory, "http://localhost:8080/hello"); } @Test @@ -102,9 +103,10 @@ public abstract class AbstractEmbeddedServletContainerFactoryTests { ConfigurableEmbeddedServletContainerFactory factory = getFactory(); this.container = factory .getEmbeddedServletContainer(exampleServletRegistration()); + start(factory); this.container.stop(); this.thrown.expect(SocketException.class); - getResponse("http://localhost:8080/hello"); + getResponse(null, "http://localhost:8080/hello"); } @Test @@ -135,7 +137,8 @@ public abstract class AbstractEmbeddedServletContainerFactoryTests { this.container = factory.getEmbeddedServletContainer( exampleServletRegistration(), new FilterRegistrationBean( new ExampleFilter())); - assertThat(getResponse("http://localhost:8080/hello"), equalTo("[Hello World]")); + assertThat(getResponse(factory, "http://localhost:8080/hello"), + equalTo("[Hello World]")); } @Test @@ -165,7 +168,8 @@ public abstract class AbstractEmbeddedServletContainerFactoryTests { factory.setPort(8081); this.container = factory .getEmbeddedServletContainer(exampleServletRegistration()); - assertThat(getResponse("http://localhost:8081/hello"), equalTo("Hello World")); + assertThat(getResponse(factory, "http://localhost:8081/hello"), + equalTo("Hello World")); } @Test @@ -174,7 +178,8 @@ public abstract class AbstractEmbeddedServletContainerFactoryTests { factory.setContextPath("/say"); this.container = factory .getEmbeddedServletContainer(exampleServletRegistration()); - assertThat(getResponse("http://localhost:8080/say/hello"), equalTo("Hello World")); + assertThat(getResponse(factory, "http://localhost:8080/say/hello"), + equalTo("Hello World")); } @Test @@ -204,6 +209,7 @@ public abstract class AbstractEmbeddedServletContainerFactoryTests { ConfigurableEmbeddedServletContainerFactory factory = getFactory(); this.container = factory .getEmbeddedServletContainer(exampleServletRegistration()); + start(factory); this.container.stop(); this.container.stop(); } @@ -232,12 +238,15 @@ public abstract class AbstractEmbeddedServletContainerFactoryTests { AbstractEmbeddedServletContainerFactory factory = getFactory(); factory.setDocumentRoot(this.temporaryFolder.getRoot()); this.container = factory.getEmbeddedServletContainer(); - assertThat(getResponse("http://localhost:8080/test.txt"), equalTo("test")); + assertThat(getResponse(factory, "http://localhost:8080/test.txt"), + equalTo("test")); } // FIXME test error page - protected String getResponse(String url) throws IOException, URISyntaxException { + protected String getResponse(EmbeddedServletContainerFactory factory, String url) + throws IOException, URISyntaxException { + start(factory); SimpleClientHttpRequestFactory clientHttpRequestFactory = new SimpleClientHttpRequestFactory(); ClientHttpRequest request = clientHttpRequestFactory.createRequest(new URI(url), HttpMethod.GET); @@ -250,6 +259,12 @@ public abstract class AbstractEmbeddedServletContainerFactoryTests { } } + private void start(EmbeddedServletContainerFactory factory) { + if (factory instanceof AbstractEmbeddedServletContainerFactory) { + ((AbstractEmbeddedServletContainerFactory) factory).onApplicationEvent(null); + } + } + protected abstract AbstractEmbeddedServletContainerFactory getFactory(); private ServletContextInitializer exampleServletRegistration() { diff --git a/spring-starters/spring-starter-parent/pom.xml b/spring-starters/spring-starter-parent/pom.xml index 18ce3a02e3..af02229291 100644 --- a/spring-starters/spring-starter-parent/pom.xml +++ b/spring-starters/spring-starter-parent/pom.xml @@ -203,6 +203,13 @@ maven-source-plugin 2.2.1 + + exec-maven-plugin + 1.2.1 + + ${start-class} + + maven-surefire-plugin 2.15