Open Network Connectors before starting contexts. #2053

gregw · 2017-12-13T09:35:15Z

Add option to open network connectors to check port availability prior to starting contexts.

Prompted by question in #2049

Signed-off-by: Greg Wilkins gregw@webtide.com

Add option to open network connectors to check port availability prior to starting contexts. Signed-off-by: Greg Wilkins <gregw@webtide.com>

gregw · 2017-12-13T09:37:50Z

@WalkerWatch this could probably use a small note in the documentation somewhere. This allows a good starting sequence:

Check ports are available and reserve them, but do not accept connections
Start contexts
Start connectors

gregw · 2017-12-13T09:43:07Z

@joakime @sbordet Should I also add an option (or use this option) to check that the contexts started OK and if not, don't start the connectors. If so, perhaps this option should be renamed (bad name anyway), to something like setVerifiedStartSequence(boolean)... in fact I shall do this rename now.... but open to feedback.

Signed-off-by: Greg Wilkins <gregw@webtide.com>

…en-network-connectors

Signed-off-by: Greg Wilkins <gregw@webtide.com>

joakime · 2017-12-13T20:52:12Z

jetty-server/src/main/config/etc/jetty.xml

@@ -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>


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?

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

joakime · 2017-12-13T20:53:48Z

jetty-server/src/main/java/org/eclipse/jetty/server/Server.java

-     * @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.


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

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

joakime · 2017-12-13T20:54:16Z

jetty-server/src/main/java/org/eclipse/jetty/server/Server.java

+     * 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>


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

joakime · 2017-12-13T20:57:23Z

jetty-server/src/main/java/org/eclipse/jetty/server/Server.java

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


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?

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....

…en-network-connectors

Signed-off-by: Greg Wilkins <gregw@webtide.com>

sbordet · 2017-12-14T09:31:25Z

@gregw I don't like the name "verifiedStartSequence" since it does not convey what it really does.
Perhaps "acceptAfterContextStart" ?

Furthermore, I would have expected this feature to build on #1732 (pause accepting) rather than calling open() before start().

gregw · 2017-12-14T13:09:43Z

@sbordet I'm not that keen on the name verifiedStartSequence, but the getter isVerifiedStartSequence returns true iff the start sequence is verified to have worked correctly, so the language works OK. Still open to suggestions for something better, but acceptAfterContextStart is not what it does... there may not even be any contexts... only handlers!

I was thinking about making it build on #1732, but that is actually more complex as it means you have to start the connectors only to pause accepting. Simpler to just open the endpoint to check the port is available (and to reserve it), but not start the connector - as we don't need a started connector to handle existing connections.

sbordet · 2017-12-14T14:19:11Z

@gregw I'm worried that people assumed start() was called before open(), while I understand now the order is reversed ?

gregw · 2017-12-14T16:37:52Z

We've had the ability to call open() before start() for a long time. When users used to hard code XML, it was not uncommon for the open() call to be put into jetty.xml or similar to check the ports were available prior to starting the contexts.

This really just an enhancement of that existing capability so that it is a) possible to do from the module system; b) a lot better on clean up in case this is used from an embedded system where the JVM does not die immediately after failure to start.

gregw · 2017-12-27T13:55:27Z

@sbordet,

here is a compromise. How about we don't merge this with 9.4, but instead merge it with jetty-10 as the only way we start the server. Ie there will be no mode, it will be just the way we start the server?

Open Network Connectors before starting contexts.

418890c

Add option to open network connectors to check port availability prior to starting contexts. Signed-off-by: Greg Wilkins <gregw@webtide.com>

gregw mentioned this pull request Dec 13, 2017

what does contextInitialized of ServletContextListener really mean? #2049

Closed

gregw requested a review from joakime December 13, 2017 09:36

gregw self-assigned this Dec 13, 2017

gregw added the Enhancement label Dec 13, 2017

gregw added this to the 9.4.x milestone Dec 13, 2017

gregw added 3 commits December 13, 2017 10:52

Rename Open Network Connectors to Verified Start Sequence

9b4429a

Signed-off-by: Greg Wilkins <gregw@webtide.com>

Merge remote-tracking branch 'origin/jetty-9.4.x' into jetty-9.4.x-op…

f077b5c

…en-network-connectors

fixed javadoc

13e008a

Signed-off-by: Greg Wilkins <gregw@webtide.com>

joakime reviewed Dec 13, 2017

View reviewed changes

gregw added 2 commits December 13, 2017 23:40

Merge remote-tracking branch 'origin/jetty-9.4.x' into jetty-9.4.x-op…

689523a

…en-network-connectors

simplification after review and better tests

fc3191a

Signed-off-by: Greg Wilkins <gregw@webtide.com>

Merge branch 'jetty-9.4.x' into jetty-9.4.x-open-network-connectors

0c7a3cf

gregw mentioned this pull request Jan 8, 2018

Server should open connectors early in start sequence #2103

Closed

gregw closed this Jan 8, 2018

gregw deleted the jetty-9.4.x-open-network-connectors branch January 8, 2018 16:16

gregw mentioned this pull request Jan 8, 2018

Issue #2103 open connectors before starting #2104

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Open Network Connectors before starting contexts. #2053

Open Network Connectors before starting contexts. #2053

gregw commented Dec 13, 2017

gregw commented Dec 13, 2017

gregw commented Dec 13, 2017

joakime Dec 13, 2017

gregw Dec 13, 2017

joakime Dec 13, 2017

gregw Dec 13, 2017

joakime Dec 13, 2017

joakime Dec 13, 2017

gregw Dec 13, 2017

sbordet commented Dec 14, 2017

gregw commented Dec 14, 2017 •

edited

Loading

sbordet commented Dec 14, 2017

gregw commented Dec 14, 2017 •

edited

Loading

gregw commented Dec 27, 2017

Open Network Connectors before starting contexts. #2053

Open Network Connectors before starting contexts. #2053

Conversation

gregw commented Dec 13, 2017

gregw commented Dec 13, 2017

gregw commented Dec 13, 2017

joakime Dec 13, 2017

Choose a reason for hiding this comment

gregw Dec 13, 2017

Choose a reason for hiding this comment

joakime Dec 13, 2017

Choose a reason for hiding this comment

gregw Dec 13, 2017

Choose a reason for hiding this comment

joakime Dec 13, 2017

Choose a reason for hiding this comment

joakime Dec 13, 2017

Choose a reason for hiding this comment

gregw Dec 13, 2017

Choose a reason for hiding this comment

sbordet commented Dec 14, 2017

gregw commented Dec 14, 2017 • edited Loading

sbordet commented Dec 14, 2017

gregw commented Dec 14, 2017 • edited Loading

gregw commented Dec 27, 2017

gregw commented Dec 14, 2017 •

edited

Loading

gregw commented Dec 14, 2017 •

edited

Loading