From 4c7e457582c00aae351e2ad9a97b495c46a71dda Mon Sep 17 00:00:00 2001 From: Madhura Bhave Date: Thu, 13 Jun 2019 15:11:18 -0700 Subject: [PATCH] Improve analysis of tomcat bind exception Using the throwOnFailure attribute on the tomcat connector, we can now determine if the underlying exception was a BindException and throw a PortInUseException instead of the generic WebServerException. Closes gh-7130 --- .../TomcatReactiveWebServerFactory.java | 1 + .../tomcat/TomcatServletWebServerFactory.java | 1 + .../web/embedded/tomcat/TomcatWebServer.java | 15 +++++++ .../JettyServletWebServerFactoryTests.java | 7 ++- .../TomcatReactiveWebServerFactoryTests.java | 45 +++++++++++++++++++ .../TomcatServletWebServerFactoryTests.java | 21 ++++----- .../UndertowServletWebServerFactoryTests.java | 7 ++- .../AbstractServletWebServerFactoryTests.java | 9 ++-- 8 files changed, 88 insertions(+), 18 deletions(-) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/TomcatReactiveWebServerFactory.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/TomcatReactiveWebServerFactory.java index 1c31379bc76..6318105b876 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/TomcatReactiveWebServerFactory.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/TomcatReactiveWebServerFactory.java @@ -115,6 +115,7 @@ public class TomcatReactiveWebServerFactory extends AbstractReactiveWebServerFac File baseDir = (this.baseDirectory != null) ? this.baseDirectory : createTempDir("tomcat"); tomcat.setBaseDir(baseDir.getAbsolutePath()); Connector connector = new Connector(this.protocol); + connector.setThrowOnFailure(true); tomcat.getService().addConnector(connector); customizeConnector(connector); tomcat.setConnector(connector); diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/TomcatServletWebServerFactory.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/TomcatServletWebServerFactory.java index 5a3b3528f22..8bd3dc7e285 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/TomcatServletWebServerFactory.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/tomcat/TomcatServletWebServerFactory.java @@ -176,6 +176,7 @@ public class TomcatServletWebServerFactory extends AbstractServletWebServerFacto File baseDir = (this.baseDirectory != null) ? this.baseDirectory : createTempDir("tomcat"); tomcat.setBaseDir(baseDir.getAbsolutePath()); Connector connector = new Connector(this.protocol); + connector.setThrowOnFailure(true); tomcat.getService().addConnector(connector); customizeConnector(connector); tomcat.setConnector(connector); 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 d87de6211d4..9aabbadb3c0 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 @@ -16,6 +16,7 @@ package org.springframework.boot.web.embedded.tomcat; +import java.net.BindException; import java.util.Arrays; import java.util.HashMap; import java.util.Map; @@ -37,6 +38,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.naming.ContextBindings; +import org.springframework.boot.web.server.PortInUseException; import org.springframework.boot.web.server.WebServer; import org.springframework.boot.web.server.WebServerException; import org.springframework.util.Assert; @@ -207,6 +209,9 @@ public class TomcatWebServer implements WebServer { throw ex; } catch (Exception ex) { + if (findBindException(ex) != null) { + throw new PortInUseException(this.tomcat.getConnector().getPort()); + } throw new WebServerException("Unable to start embedded Tomcat server", ex); } finally { @@ -229,6 +234,16 @@ public class TomcatWebServer implements WebServer { } } + private BindException findBindException(Throwable ex) { + if (ex == null) { + return null; + } + if (ex instanceof BindException) { + return (BindException) ex; + } + return findBindException(ex.getCause()); + } + private void stopSilently() { try { stopTomcat(); 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 83ae74e836c..c78436406b1 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 @@ -338,9 +338,14 @@ class JettyServletWebServerFactoryTests extends AbstractServletWebServerFactoryT } @Override - protected void handleExceptionCausedByBlockedPort(RuntimeException ex, int blockedPort) { + protected void handleExceptionCausedByBlockedPortOnPrimaryConnector(RuntimeException ex, int blockedPort) { assertThat(ex).isInstanceOf(PortInUseException.class); assertThat(((PortInUseException) ex).getPort()).isEqualTo(blockedPort); } + @Override + protected void handleExceptionCausedByBlockedPortOnSecondaryConnector(RuntimeException ex, int blockedPort) { + this.handleExceptionCausedByBlockedPortOnPrimaryConnector(ex, blockedPort); + } + } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/tomcat/TomcatReactiveWebServerFactoryTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/tomcat/TomcatReactiveWebServerFactoryTests.java index f207050aedf..348580b685a 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/tomcat/TomcatReactiveWebServerFactoryTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/embedded/tomcat/TomcatReactiveWebServerFactoryTests.java @@ -16,6 +16,9 @@ package org.springframework.boot.web.embedded.tomcat; +import java.io.IOException; +import java.net.InetSocketAddress; +import java.net.ServerSocket; import java.util.Arrays; import org.apache.catalina.Context; @@ -32,10 +35,15 @@ import org.junit.jupiter.api.Test; import org.mockito.ArgumentCaptor; import org.mockito.InOrder; +import org.springframework.boot.web.reactive.server.AbstractReactiveWebServerFactory; import org.springframework.boot.web.reactive.server.AbstractReactiveWebServerFactoryTests; +import org.springframework.boot.web.server.PortInUseException; +import org.springframework.boot.web.servlet.server.AbstractServletWebServerFactoryTests; import org.springframework.http.server.reactive.HttpHandler; +import org.springframework.util.SocketUtils; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.inOrder; @@ -188,4 +196,41 @@ class TomcatReactiveWebServerFactoryTests extends AbstractReactiveWebServerFacto assertThat(context.getClearReferencesThreadLocals()).isFalse(); } + @Test + protected void portClashOfPrimaryConnectorResultsInPortInUseException() throws IOException { + doWithBlockedPort((port) -> { + assertThatExceptionOfType(RuntimeException.class).isThrownBy(() -> { + AbstractReactiveWebServerFactory factory = getFactory(); + factory.setPort(port); + this.webServer = factory.getWebServer(mock(HttpHandler.class)); + this.webServer.start(); + }).satisfies((ex) -> handleExceptionCausedByBlockedPortOnPrimaryConnector(ex, port)); + }); + } + + protected final void doWithBlockedPort(AbstractServletWebServerFactoryTests.BlockedPortAction action) + throws IOException { + int port = SocketUtils.findAvailableTcpPort(40000); + ServerSocket serverSocket = new ServerSocket(); + for (int i = 0; i < 10; i++) { + try { + serverSocket.bind(new InetSocketAddress(port)); + break; + } + catch (Exception ex) { + } + } + try { + action.run(port); + } + finally { + serverSocket.close(); + } + } + + protected void handleExceptionCausedByBlockedPortOnPrimaryConnector(RuntimeException ex, int blockedPort) { + assertThat(ex).isInstanceOf(PortInUseException.class); + assertThat(((PortInUseException) ex).getPort()).isEqualTo(blockedPort); + } + } 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 8887e1c6c34..44456c0f143 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 @@ -66,6 +66,7 @@ import org.mockito.ArgumentCaptor; import org.mockito.InOrder; import org.springframework.boot.testsupport.system.CapturedOutput; +import org.springframework.boot.web.server.PortInUseException; import org.springframework.boot.web.server.WebServerException; import org.springframework.boot.web.servlet.server.AbstractServletWebServerFactory; import org.springframework.boot.web.servlet.server.AbstractServletWebServerFactoryTests; @@ -326,18 +327,6 @@ class TomcatServletWebServerFactoryTests extends AbstractServletWebServerFactory assertThat(connector.getURIEncoding()).isEqualTo("UTF-8"); } - @Test - void primaryConnectorPortClashThrowsWebServerException() throws IOException { - doWithBlockedPort((port) -> { - TomcatServletWebServerFactory factory = getFactory(); - factory.setPort(port); - assertThatExceptionOfType(WebServerException.class).isThrownBy(() -> { - this.webServer = factory.getWebServer(); - this.webServer.start(); - }); - }); - } - @Test void startupFailureDoesNotResultInUnstoppedThreadsBeingReported(CapturedOutput capturedOutput) throws IOException { super.portClashOfPrimaryConnectorResultsInPortInUseException(); @@ -593,7 +582,13 @@ class TomcatServletWebServerFactoryTests extends AbstractServletWebServerFactory } @Override - protected void handleExceptionCausedByBlockedPort(RuntimeException ex, int blockedPort) { + protected void handleExceptionCausedByBlockedPortOnPrimaryConnector(RuntimeException ex, int blockedPort) { + assertThat(ex).isInstanceOf(PortInUseException.class); + assertThat(((PortInUseException) ex).getPort()).isEqualTo(blockedPort); + } + + @Override + protected void handleExceptionCausedByBlockedPortOnSecondaryConnector(RuntimeException ex, int blockedPort) { assertThat(ex).isInstanceOf(ConnectorStartFailedException.class); assertThat(((ConnectorStartFailedException) ex).getPort()).isEqualTo(blockedPort); } 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 d9e9ec9621f..c7df2f26952 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 @@ -279,11 +279,16 @@ class UndertowServletWebServerFactoryTests extends AbstractServletWebServerFacto } @Override - protected void handleExceptionCausedByBlockedPort(RuntimeException ex, int blockedPort) { + protected void handleExceptionCausedByBlockedPortOnPrimaryConnector(RuntimeException ex, int blockedPort) { assertThat(ex).isInstanceOf(PortInUseException.class); assertThat(((PortInUseException) ex).getPort()).isEqualTo(blockedPort); Undertow undertow = (Undertow) ReflectionTestUtils.getField(this.webServer, "undertow"); assertThat(undertow.getWorker()).isNull(); } + @Override + protected void handleExceptionCausedByBlockedPortOnSecondaryConnector(RuntimeException ex, int blockedPort) { + this.handleExceptionCausedByBlockedPortOnPrimaryConnector(ex, blockedPort); + } + } 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 3ad43aa3647..1cbb13ce320 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 @@ -831,7 +831,7 @@ public abstract class AbstractServletWebServerFactoryTests { factory.setPort(port); AbstractServletWebServerFactoryTests.this.webServer = factory.getWebServer(); AbstractServletWebServerFactoryTests.this.webServer.start(); - }).satisfies((ex) -> handleExceptionCausedByBlockedPort(ex, port)); + }).satisfies((ex) -> handleExceptionCausedByBlockedPortOnPrimaryConnector(ex, port)); }); } @@ -843,7 +843,7 @@ public abstract class AbstractServletWebServerFactoryTests { addConnector(port, factory); AbstractServletWebServerFactoryTests.this.webServer = factory.getWebServer(); AbstractServletWebServerFactoryTests.this.webServer.start(); - }).satisfies((ex) -> handleExceptionCausedByBlockedPort(ex, port)); + }).satisfies((ex) -> handleExceptionCausedByBlockedPortOnSecondaryConnector(ex, port)); }); } @@ -962,7 +962,10 @@ public abstract class AbstractServletWebServerFactoryTests { protected abstract void addConnector(int port, AbstractServletWebServerFactory factory); - protected abstract void handleExceptionCausedByBlockedPort(RuntimeException ex, int blockedPort); + protected abstract void handleExceptionCausedByBlockedPortOnPrimaryConnector(RuntimeException ex, int blockedPort); + + protected abstract void handleExceptionCausedByBlockedPortOnSecondaryConnector(RuntimeException ex, + int blockedPort); private boolean doTestCompression(int contentSize, String[] mimeTypes, String[] excludedUserAgents) throws Exception {