From c420249f7de37ef630a54b6c882489ac0e78014d Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Tue, 23 Sep 2014 11:51:24 +0100 Subject: [PATCH] Detect startup failures in additional Tomcat connectors Previously, only the state of the primary connector was checked when verifying that the embedded Tomcat instance has started successfully. This commit updates the verification logic to examine all of the service's connectors, thereby also detecting startup failures of any additional connectors the have been configured. A check on the primary connector's state has been removed from initialize as, at this stage, its state will always be NEW. Fixes gh-1591 --- .../TomcatEmbeddedServletContainer.java | 18 +++-- ...tEmbeddedServletContainerFactoryTests.java | 74 +++++++++++++++++++ 2 files changed, 86 insertions(+), 6 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 0965e9e9d6..d60a98fbc2 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,11 +90,6 @@ public class TomcatEmbeddedServletContainer implements EmbeddedServletContainer // Unlike Jetty, all Tomcat threads are daemon threads. We create a // blocking non-daemon to stop immediate shutdown startDaemonAwaitThread(); - - if (LifecycleState.FAILED.equals(this.tomcat.getConnector().getState())) { - this.tomcat.stop(); - throw new IllegalStateException("Tomcat connector in failed state"); - } } catch (Exception ex) { throw new EmbeddedServletContainerException( @@ -151,13 +146,24 @@ public class TomcatEmbeddedServletContainer implements EmbeddedServletContainer if (connector != null && this.autoStart) { startConnector(connector); } + // Ensure process isn't left running if it actually failed to start - if (LifecycleState.FAILED.equals(this.tomcat.getConnector().getState())) { + + if (connectorsHaveFailedToStart()) { stopSilently(); throw new IllegalStateException("Tomcat connector in failed state"); } } + private boolean connectorsHaveFailedToStart() { + for (Connector connector : this.tomcat.getService().findConnectors()) { + if (LifecycleState.FAILED.equals(connector.getState())) { + return true; + } + } + return false; + } + private void stopSilently() { try { this.tomcat.stop(); 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 a5071c61f8..b2bbcb4e11 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 @@ -16,6 +16,10 @@ package org.springframework.boot.context.embedded.tomcat; +import java.io.IOException; +import java.net.InetSocketAddress; +import java.net.ServerSocket; +import java.net.UnknownHostException; import java.util.Arrays; import java.util.Map; import java.util.concurrent.TimeUnit; @@ -39,6 +43,7 @@ import static org.hamcrest.Matchers.equalTo; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThat; +import static org.junit.Assert.fail; import static org.mockito.BDDMockito.given; import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyObject; @@ -248,6 +253,63 @@ public class TomcatEmbeddedServletContainerFactoryTests extends assertThat(jsseProtocol.getCiphers(), equalTo("ALPHA,BRAVO,CHARLIE")); } + @Test + public void primaryConnectorPortClashThrowsIllegalStateException() + throws InterruptedException, UnknownHostException, IOException { + final int port = SocketUtils.findAvailableTcpPort(40000); + + doWithBlockedPort(port, new Runnable() { + + @Override + public void run() { + TomcatEmbeddedServletContainerFactory factory = getFactory(); + factory.setPort(port); + + try { + TomcatEmbeddedServletContainerFactoryTests.this.container = factory + .getEmbeddedServletContainer(); + TomcatEmbeddedServletContainerFactoryTests.this.container.start(); + fail(); + } + catch (IllegalStateException ex) { + + } + } + + }); + + } + + @Test + public void additionalConnectorPortClashThrowsIllegalStateException() + throws InterruptedException, UnknownHostException, IOException { + final int port = SocketUtils.findAvailableTcpPort(40000); + + doWithBlockedPort(port, new Runnable() { + + @Override + public void run() { + TomcatEmbeddedServletContainerFactory factory = getFactory(); + Connector connector = new Connector( + "org.apache.coyote.http11.Http11NioProtocol"); + connector.setPort(port); + factory.addAdditionalTomcatConnectors(connector); + + try { + TomcatEmbeddedServletContainerFactoryTests.this.container = factory + .getEmbeddedServletContainer(); + TomcatEmbeddedServletContainerFactoryTests.this.container.start(); + fail(); + } + catch (IllegalStateException ex) { + + } + } + + }); + + } + private void assertTimeout(TomcatEmbeddedServletContainerFactory factory, int expected) { Tomcat tomcat = getTomcat(factory); Context context = (Context) tomcat.getHost().findChildren()[0]; @@ -259,4 +321,16 @@ public class TomcatEmbeddedServletContainerFactoryTests extends return ((TomcatEmbeddedServletContainer) this.container).getTomcat(); } + private void doWithBlockedPort(final int port, Runnable action) throws IOException { + ServerSocket serverSocket = new ServerSocket(); + serverSocket.bind(new InetSocketAddress(port)); + + try { + action.run(); + } + finally { + serverSocket.close(); + } + } + }