From 418890c855601067c478b54ee9aec6ccaf1f06fc Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 13 Dec 2017 10:33:14 +0100 Subject: [PATCH 1/4] Open Network Connectors before starting contexts. Add option to open network connectors to check port availability prior to starting contexts. Signed-off-by: Greg Wilkins --- jetty-server/src/main/config/etc/jetty.xml | 1 + .../src/main/config/modules/server.mod | 3 + .../java/org/eclipse/jetty/server/Server.java | 50 ++++++++++++ .../jetty/server/NotAcceptingTest.java | 76 +++++++++++++++++++ 4 files changed, 130 insertions(+) diff --git a/jetty-server/src/main/config/etc/jetty.xml b/jetty-server/src/main/config/etc/jetty.xml index 613170643d76..45d1eb1fb5b5 100644 --- a/jetty-server/src/main/config/etc/jetty.xml +++ b/jetty-server/src/main/config/etc/jetty.xml @@ -126,5 +126,6 @@ + diff --git a/jetty-server/src/main/config/modules/server.mod b/jetty-server/src/main/config/modules/server.mod index a15eef1ea9d3..cfcdcd48df6b 100644 --- a/jetty-server/src/main/config/modules/server.mod +++ b/jetty-server/src/main/config/modules/server.mod @@ -82,3 +82,6 @@ etc/jetty.xml ## Dump the state of the Jetty server, components, and webapps before shutdown # jetty.server.dumpBeforeStop=false + +## Open Network Connectors before starting contexts +# jetty.server.openNetworkConnectors=false diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java index ef2549202563..c59c70873c27 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java @@ -85,6 +85,7 @@ public class Server extends HandlerWrapper implements Attributes private boolean _stopAtShutdown; private boolean _dumpAfterStart=false; private boolean _dumpBeforeStop=false; + private boolean _openNetworkConnectors=false; private ErrorHandler _errorHandler; private RequestLog _requestLog; @@ -136,6 +137,27 @@ public Server(@Name("threadpool") ThreadPool pool) setServer(this); } + /** + * @return True if {@link NetworkConnector}s are opened with {@link NetworkConnector#open()} during + * start before handlers are started. + */ + public boolean getOpenNetworkConnectors() + { + return _openNetworkConnectors; + } + + /** + * Set if {@link NetworkConnector}s are opened with {@link NetworkConnector#open()} before starting + * contained {@link Handler}s including contexts. If any failures are encountered, then the start + * will be failed without starting any handlers. + * @param openNetworkConnectors If true {@link NetworkConnector}s are opened with {@link NetworkConnector#open()} + * during start before handlers are started. + */ + public void setOpenNetworkConnectors(boolean openNetworkConnectors) + { + _openNetworkConnectors = openNetworkConnectors; + } + /* ------------------------------------------------------------ */ public RequestLog getRequestLog() { @@ -380,8 +402,36 @@ protected void doStart() throws Exception HttpGenerator.setJettyVersion(HttpConfiguration.SERVER_VERSION); MultiException mex=new MultiException(); + + + // Open network connectors first + if (_openNetworkConnectors) + { + _connectors.stream().filter(NetworkConnector.class::isInstance).map(NetworkConnector.class::cast).forEach(connector-> + { + try + { + connector.open(); + } + catch(Throwable e) + { + mex.add(e); + } + }); + + if (mex.size()>0) + { + // Close any that were open + _connectors.stream().filter(NetworkConnector.class::isInstance).map(NetworkConnector.class::cast).forEach(NetworkConnector::close); + mex.ifExceptionThrow(); + } + } + try { + // Start the server and components, but #start(LifeCycle) is overridden + // so that connectors are not started until after all other components are + // started. super.doStart(); } catch(Throwable e) diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/NotAcceptingTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/NotAcceptingTest.java index 4574604f6d43..75dcf6c89489 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/NotAcceptingTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/NotAcceptingTest.java @@ -18,14 +18,21 @@ package org.eclipse.jetty.server; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.is; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThat; import java.io.IOException; +import java.net.BindException; +import java.net.ServerSocket; import java.net.Socket; import java.util.concurrent.Exchanger; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; @@ -376,6 +383,75 @@ public void testConnectionLimit() throws Exception assertThat(asyncConnector.isAccepting(),is(true)); } + + @Test + public void testOpenBeforeStart() throws Exception + { + server.setOpenNetworkConnectors(true); + + try(ServerSocket alreadyOpened = new ServerSocket(0)) + { + // Add a connector that can be opened + AtomicInteger opened0 = new AtomicInteger(0); + ServerConnector connector0 = new ServerConnector(server) + { + @Override + public void open() throws IOException + { + super.open(); + opened0.set(getLocalPort()); + } + + }; + server.addConnector(connector0); + + // Add a connector that will fail to open with port in use + ServerConnector connector1 = new ServerConnector(server); + connector1.setPort(alreadyOpened.getLocalPort()); + server.addConnector(connector1); + + + // Add a handler to detect if handlers are started + AtomicBoolean handlerStarted = new AtomicBoolean(false); + server.setHandler(new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) + throws IOException, ServletException + { + } + + @Override + public void doStart() + { + handlerStarted.set(true); + } + }); + + + // try to start the server + try + { + server.start(); + Assert.fail(); + } + catch (BindException e) + { + // expected + assertThat(e.getMessage(),containsString("Address already in use")); + } + + // Check that connector0 was opened OK + assertThat(opened0.get(),greaterThan(0)); + // and it is now closed + assertFalse(connector0.isOpen()); + assertFalse(connector0.isRunning()); + + // Check that handlers were never started + assertFalse(handlerStarted.get()); + + } + } public static class HelloHandler extends AbstractHandler { From 9b4429ab179fc8b57142b86ad64720adf475eee8 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 13 Dec 2017 10:52:52 +0100 Subject: [PATCH 2/4] Rename Open Network Connectors to Verified Start Sequence Signed-off-by: Greg Wilkins --- jetty-server/src/main/config/etc/jetty.xml | 2 +- .../src/main/config/modules/server.mod | 4 +- .../java/org/eclipse/jetty/server/Server.java | 183 ++++++++++-------- .../jetty/server/NotAcceptingTest.java | 5 +- 4 files changed, 105 insertions(+), 89 deletions(-) diff --git a/jetty-server/src/main/config/etc/jetty.xml b/jetty-server/src/main/config/etc/jetty.xml index 45d1eb1fb5b5..7624949b7e1a 100644 --- a/jetty-server/src/main/config/etc/jetty.xml +++ b/jetty-server/src/main/config/etc/jetty.xml @@ -126,6 +126,6 @@ - + diff --git a/jetty-server/src/main/config/modules/server.mod b/jetty-server/src/main/config/modules/server.mod index cfcdcd48df6b..5264388bfab4 100644 --- a/jetty-server/src/main/config/modules/server.mod +++ b/jetty-server/src/main/config/modules/server.mod @@ -83,5 +83,5 @@ etc/jetty.xml ## Dump the state of the Jetty server, components, and webapps before shutdown # jetty.server.dumpBeforeStop=false -## Open Network Connectors before starting contexts -# jetty.server.openNetworkConnectors=false +## Verified Start Sequence by checking ports and contexts are available before starting connectors +# jetty.server.verifiedStartSequence=false diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java index c59c70873c27..4084c43a16ba 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java @@ -85,7 +85,7 @@ public class Server extends HandlerWrapper implements Attributes private boolean _stopAtShutdown; private boolean _dumpAfterStart=false; private boolean _dumpBeforeStop=false; - private boolean _openNetworkConnectors=false; + private boolean _verifiedStartSequence=false; private ErrorHandler _errorHandler; private RequestLog _requestLog; @@ -138,24 +138,30 @@ public Server(@Name("threadpool") ThreadPool pool) } /** - * @return True if {@link NetworkConnector}s are opened with {@link NetworkConnector#open()} during - * start before handlers are started. + * @return True if the start sequence is verified by checking port availability prior + * to starting contexts and checking contexts prior to starting connectors. */ - public boolean getOpenNetworkConnectors() + public boolean isVerifiedStartSequence() { - return _openNetworkConnectors; + return _verifiedStartSequence; } /** - * Set if {@link NetworkConnector}s are opened with {@link NetworkConnector#open()} before starting - * contained {@link Handler}s including contexts. If any failures are encountered, then the start - * will be failed without starting any handlers. - * @param openNetworkConnectors If true {@link NetworkConnector}s are opened with {@link NetworkConnector#open()} - * during start before handlers are started. + * Set if the start sequence is to be verified by in the following stages: + * + *
  • {@link NetworkConnector}s are opened with {@link NetworkConnector#open()} to check ports are + * available and to reserve them.
    • + *
  • Start the contained {@link Handler}s and beans.
    • + *
  • Start the {@link Connector}s + * + * If any failures are encountered at any of the stages, the start sequence is aborted without + * running the subsequent stage and reversing closing any open connectors. + * @param verified If true the start sequence is verified by checking port availability prior + * to starting contexts and checking contexts prior to starting connectors. */ - public void setOpenNetworkConnectors(boolean openNetworkConnectors) + public void setVerifiedStartSequence(boolean verified) { - _openNetworkConnectors = openNetworkConnectors; + _verifiedStartSequence = verified; } /* ------------------------------------------------------------ */ @@ -368,96 +374,107 @@ public HttpField getDateField() @Override protected void doStart() throws Exception { - // Create an error handler if there is none - if (_errorHandler==null) - _errorHandler=getBean(ErrorHandler.class); - if (_errorHandler==null) - setErrorHandler(new ErrorHandler()); - if (_errorHandler instanceof ErrorHandler.ErrorPageMapper) - LOG.warn("ErrorPageMapper not supported for Server level Error Handling"); - _errorHandler.setServer(this); - - //If the Server should be stopped when the jvm exits, register - //with the shutdown handler thread. - if (getStopAtShutdown()) - ShutdownThread.register(this); - - //Register the Server with the handler thread for receiving - //remote stop commands - ShutdownMonitor.register(this); - - //Start a thread waiting to receive "stop" commands. - ShutdownMonitor.getInstance().start(); // initialize - - String gitHash = Jetty.GIT_HASH; - String timestamp = Jetty.BUILD_TIMESTAMP; - - LOG.info("jetty-{}, build timestamp: {}, git hash: {}", getVersion(), timestamp, gitHash); - if (!Jetty.STABLE) + try { - LOG.warn("THIS IS NOT A STABLE RELEASE! DO NOT USE IN PRODUCTION!"); - LOG.warn("Download a stable release from http://download.eclipse.org/jetty/"); - } - - HttpGenerator.setJettyVersion(HttpConfiguration.SERVER_VERSION); + // Create an error handler if there is none + if (_errorHandler==null) + _errorHandler=getBean(ErrorHandler.class); + if (_errorHandler==null) + setErrorHandler(new ErrorHandler()); + if (_errorHandler instanceof ErrorHandler.ErrorPageMapper) + LOG.warn("ErrorPageMapper not supported for Server level Error Handling"); + _errorHandler.setServer(this); + + //If the Server should be stopped when the jvm exits, register + //with the shutdown handler thread. + if (getStopAtShutdown()) + ShutdownThread.register(this); + + //Register the Server with the handler thread for receiving + //remote stop commands + ShutdownMonitor.register(this); + + //Start a thread waiting to receive "stop" commands. + ShutdownMonitor.getInstance().start(); // initialize + + String gitHash = Jetty.GIT_HASH; + String timestamp = Jetty.BUILD_TIMESTAMP; + + LOG.info("jetty-{}, build timestamp: {}, git hash: {}", getVersion(), timestamp, gitHash); + if (!Jetty.STABLE) + { + LOG.warn("THIS IS NOT A STABLE RELEASE! DO NOT USE IN PRODUCTION!"); + LOG.warn("Download a stable release from http://download.eclipse.org/jetty/"); + } - MultiException mex=new MultiException(); - + HttpGenerator.setJettyVersion(HttpConfiguration.SERVER_VERSION); - // Open network connectors first - if (_openNetworkConnectors) - { - _connectors.stream().filter(NetworkConnector.class::isInstance).map(NetworkConnector.class::cast).forEach(connector-> + MultiException mex=new MultiException(); + + // Open network connectors first + if (_verifiedStartSequence) { - try + _connectors.stream().filter(NetworkConnector.class::isInstance).map(NetworkConnector.class::cast).forEach(connector-> { - connector.open(); - } - catch(Throwable e) + try + { + connector.open(); + } + catch(Throwable e) + { + mex.add(e); + } + }); + + if (mex.size()>0) { - mex.add(e); + // Close any that were open + _connectors.stream().filter(NetworkConnector.class::isInstance).map(NetworkConnector.class::cast).forEach(NetworkConnector::close); + mex.ifExceptionThrow(); } - }); - - if (mex.size()>0) - { - // Close any that were open - _connectors.stream().filter(NetworkConnector.class::isInstance).map(NetworkConnector.class::cast).forEach(NetworkConnector::close); - mex.ifExceptionThrow(); } - } - - try - { - // Start the server and components, but #start(LifeCycle) is overridden - // so that connectors are not started until after all other components are - // started. - super.doStart(); - } - catch(Throwable e) - { - mex.add(e); - } - // start connectors last - for (Connector connector : _connectors) - { + if (_verifiedStartSequence) + mex.ifExceptionThrow(); + try { - connector.start(); + // Start the server and components, but #start(LifeCycle) is overridden + // so that connectors are not started until after all other components are + // started. + super.doStart(); } catch(Throwable e) { mex.add(e); } - } - if (isDumpAfterStart()) - dumpStdErr(); + // Throw now if verified start sequence + if (_verifiedStartSequence) + mex.ifExceptionThrow(); + + // start connectors last + for (Connector connector : _connectors) + { + try + { + connector.start(); + } + catch(Throwable e) + { + mex.add(e); + } + } - mex.ifExceptionThrow(); + mex.ifExceptionThrow(); + LOG.info(String.format("Started @%dms",Uptime.getUptime())); + } + finally + { + if (isDumpAfterStart()) + dumpStdErr(); + } - LOG.info(String.format("Started @%dms",Uptime.getUptime())); } @Override diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/NotAcceptingTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/NotAcceptingTest.java index 75dcf6c89489..1015421d9e59 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/NotAcceptingTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/NotAcceptingTest.java @@ -385,9 +385,9 @@ public void testConnectionLimit() throws Exception } @Test - public void testOpenBeforeStart() throws Exception + public void testVerifiedStartSequence() throws Exception { - server.setOpenNetworkConnectors(true); + server.setVerifiedStartSequence(true); try(ServerSocket alreadyOpened = new ServerSocket(0)) { @@ -410,7 +410,6 @@ public void open() throws IOException connector1.setPort(alreadyOpened.getLocalPort()); server.addConnector(connector1); - // Add a handler to detect if handlers are started AtomicBoolean handlerStarted = new AtomicBoolean(false); server.setHandler(new AbstractHandler() From 13e008af3ae0f0cacaa9e8be54360643b5b3fc54 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 13 Dec 2017 13:49:02 +0100 Subject: [PATCH 3/4] fixed javadoc Signed-off-by: Greg Wilkins --- .../src/main/java/org/eclipse/jetty/server/Server.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java index 4084c43a16ba..1ae7d600a797 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java @@ -148,12 +148,12 @@ public boolean isVerifiedStartSequence() /** * Set if the start sequence is to be verified by in the following stages: - * + *
      *
    • {@link NetworkConnector}s are opened with {@link NetworkConnector#open()} to check ports are * available and to reserve them.
      • *
    • Start the contained {@link Handler}s and beans.
      • *
    • Start the {@link Connector}s - * + *
    * If any failures are encountered at any of the stages, the start sequence is aborted without * running the subsequent stage and reversing closing any open connectors. * @param verified If true the start sequence is verified by checking port availability prior From fc3191aee1fac5f1617120920c57774342503bb5 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Thu, 14 Dec 2017 00:07:57 +0100 Subject: [PATCH 4/4] simplification after review and better tests Signed-off-by: Greg Wilkins --- .../java/org/eclipse/jetty/server/Server.java | 39 +++++++---- .../jetty/server/NotAcceptingTest.java | 65 ++++++++++++++++++- 2 files changed, 90 insertions(+), 14 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java index 1ae7d600a797..df58c7d93830 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java @@ -426,16 +426,9 @@ protected void doStart() throws Exception } }); - if (mex.size()>0) - { - // Close any that were open - _connectors.stream().filter(NetworkConnector.class::isInstance).map(NetworkConnector.class::cast).forEach(NetworkConnector::close); - mex.ifExceptionThrow(); - } - } - - if (_verifiedStartSequence) + // Throw now if verified start sequence and there was an open exception mex.ifExceptionThrow(); + } try { @@ -447,11 +440,11 @@ protected void doStart() throws Exception catch(Throwable e) { mex.add(e); + + // Throw now if verified start sequence and there was a doStart exception + if (_verifiedStartSequence) + mex.ifExceptionThrow(); } - - // Throw now if verified start sequence - if (_verifiedStartSequence) - mex.ifExceptionThrow(); // start connectors last for (Connector connector : _connectors) @@ -469,6 +462,26 @@ protected void doStart() throws Exception mex.ifExceptionThrow(); LOG.info(String.format("Started @%dms",Uptime.getUptime())); } + catch(Throwable x) + { + if (_verifiedStartSequence) + { + // stop any connectors that were opened + _connectors.stream().filter(NetworkConnector.class::isInstance).map(NetworkConnector.class::cast).forEach(NetworkConnector::close); + + // Stop our components (including connectors + try + { + super.doStop(); + } + catch(Throwable e) + { + x.addSuppressed(e); + } + } + + throw x; + } finally { if (isDumpAfterStart()) diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/NotAcceptingTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/NotAcceptingTest.java index 1015421d9e59..ebf9045d88d1 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/NotAcceptingTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/NotAcceptingTest.java @@ -43,6 +43,7 @@ import org.eclipse.jetty.server.handler.AbstractHandler; import org.eclipse.jetty.toolchain.test.AdvancedRunner; import org.eclipse.jetty.util.BufferUtil; +import org.eclipse.jetty.util.thread.QueuedThreadPool; import org.junit.After; import org.junit.Assert; import org.junit.Before; @@ -385,7 +386,7 @@ public void testConnectionLimit() throws Exception } @Test - public void testVerifiedStartSequence() throws Exception + public void testVerifiedStartSequenceAlreadyOpened() throws Exception { server.setVerifiedStartSequence(true); @@ -449,8 +450,70 @@ public void doStart() // Check that handlers were never started assertFalse(handlerStarted.get()); + // Check that server components were stopped + assertFalse(server.getBean(QueuedThreadPool.class).isStarted()); } } + + @Test + public void testVerifiedStartSequenceBadHandler() throws Exception + { + server.setVerifiedStartSequence(true); + + // Add a connector that can be opened + AtomicInteger opened = new AtomicInteger(0); + ServerConnector connector = new ServerConnector(server) + { + @Override + public void open() throws IOException + { + super.open(); + opened.set(getLocalPort()); + } + + }; + server.addConnector(connector); + + + // Add a handler that will fail to start + server.setHandler(new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) + throws IOException, ServletException + { + } + + @Override + public void doStart() throws Exception + { + throw new Exception("Expected failure during start"); + } + }); + + + // try to start the server + try + { + server.start(); + Assert.fail(); + } + catch (Exception e) + { + // expected + assertThat(e.getMessage(),containsString("Expected failure during start")); + } + + // Check that connector0 was opened OK + assertThat(opened.get(),greaterThan(0)); + // and it is now closed + assertFalse(connector.isOpen()); + assertFalse(connector.isRunning()); + + // Check that server components were stopped + assertFalse(server.getBean(QueuedThreadPool.class).isStarted()); + } + public static class HelloHandler extends AbstractHandler {