Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open Network Connectors before starting contexts. #2053

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion jetty-server/src/main/config/etc/jetty.xml
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,6 @@
<Set name="stopTimeout"><Property name="jetty.server.stopTimeout" default="5000"/></Set>
<Set name="dumpAfterStart"><Property name="jetty.server.dumpAfterStart" deprecated="jetty.dump.start" default="false"/></Set>
<Set name="dumpBeforeStop"><Property name="jetty.server.dumpBeforeStop" deprecated="jetty.dump.stop" default="false"/></Set>
<Set name="openNetworkConnectors"><Property name="jetty.server.openNetworkConnectors" default="false"/></Set>
<Set name="verifiedStartSequence"><Property name="jetty.server.verifiedStartSequence" default="false"/></Set>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How many versions of jetty have had the older name?
Do we want to break this kind of backward compat in 9.4.x mid-stream? or wait for jetty 10.x?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

none - the "old" name was new in this pull request.


</Configure>
4 changes: 2 additions & 2 deletions jetty-server/src/main/config/modules/server.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
183 changes: 100 additions & 83 deletions jetty-server/src/main/java/org/eclipse/jetty/server/Server.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This description is awkward to me.
The prior version was more clear.

Are we binding or not?
What about windows and overlapping bind? (where windows allows you to bind to a port already bound by something else without error?) /me shakes fist at IIS

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is now doing a bit more, so I put a longer description on the setter, which I hope is clear?

*/
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:
* <nl>
* <li>{@link NetworkConnector}s are opened with {@link NetworkConnector#open()} to check ports are
* available and to reserve them.</li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you reserving them? or binding them?
This description is awkward (to me)

* <li>Start the contained {@link Handler}s and beans.</li>
* <li>Start the {@link Connector}s
* </nl>
* 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;
}

/* ------------------------------------------------------------ */
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if something above (not in a try/catch of its own) causes an error?
Such as if ShutdownMonitor.register(this) had an exception?
This wouldn't be logged / dumped anywhere, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any other exception would still be thrown as an exception from the original call to start() and the state would end up as FAILED. These happen before we open connectors, so that is OK.

However, I think that we should also close any opened connectors if a context fails....

{
if (isDumpAfterStart())
dumpStdErr();
}

LOG.info(String.format("Started @%dms",Uptime.getUptime()));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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))
{
Expand All @@ -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()
Expand Down