Issue #7344 - wait for forked jetty process #7374

janbartel · 2022-01-13T00:32:54Z

Change org.eclipse.jetty.server.ShutdownMonitor to add a new command: pid. This reports the process id of the running jetty process. Changed the org.eclipse.jetty.maven.plugin.JettyStopMojo to first send this command to the running jetty process to obtain its id before sending a stop command. If the pid is retrievable, the stop mojo will wait (a configurable amount of time) for the jetty process to exit before terminating. If the pid is bad or missing, then the stop mojo falls back to it's previous behaviour of waiting a configurable amount of time to receive the Stopped message from the jetty process before terminating.

Signed-off-by: Jan Bartel <[email protected]>

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyStopMojo.java

sbordet · 2022-01-14T12:04:20Z

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyStopMojo.java

        {
-            getLog().error(e);
+            out.write(command.getBytes());
+            out.flush();


No, TCP is not very friendly with open/write/close.
You should always read to have a confirmation that the other peer received the data (even if it's only reading -1).
The other machine could be overloaded, so SYN+PSH+FIN may be received by the recipient at OS level (but not yet at application level), but because the machine is overloaded timeouts may happen, so the OS decides to RST back to the sender, which has already closed.
The net result is that the sender does not know that something went wrong, the recipient never received the data at the application level, and everything went into /dev/null.

If the sender is interested in getting confirmation from the remote jetty, then they configure a non-zero stopWait timeout, in which case we set the socket soTimeout and read from it to get the "Shutting down" message. Alternatively, if we have been able to obtain the pid, we send the message and then wait up to stopWait seconds on the process itself to stop. So in either case, there will be a wait for either a response message, or the process to die. If no wait is configured, then we don't wait for any response, we just send the message and exit.

You are assuming that the message you send reaches the other end. I'm saying that it may not.
Now, if you care whether the message reaches the other end, you should read, at least a -1.
If you don't care whether the message reaches the other end, then you may not read (like the code above), just don't assume that send() actually produces the results you want.
It's not about getting confirmation about the specific command.
It's about getting a confirmation that the message even arrived to the other process.

So @sbordet how long does the sender need to wait to read from the socket to ensure that the message was sent? The behaviour of the ShutdownMonitor is to stop jetty before replying with "Stopped" and exiting. In the case where the caller has not configured a stopWait time they do not want to wait until the socket is closed etc.

sbordet · 2022-01-14T12:14:40Z

jetty-maven-plugin/src/test/java/org/eclipse/jetty/maven/plugin/MockShutdownMonitor.java

+ * org.eclipse.jetty.server.ShutdownMonitor.
+ *
+ */
+public class MockShutdownMonitor


Why can't we use ShutdownMonitor?
It should be possible because there are tests that use ShutdownMonitor directly and not a mock.
We really want to test the real ShutdownMonitor, not this mock class.

@sbordet I'm not interested in testing ShutdownMonitor - we have other tests for that ;). The unit test it relates to is of the JettyStopMojo - all I want to do is to test the behaviour of the JettyStopMojo, and for that I need to control the behaviour of the ShutdownMonitor and its ShutdownMonitorRunnable. I can't do that by subclassing because ShutdownMonitor is effectively not extensible, hence the mocks.

I made ShutdownMonitor.start() and stop() public and I can rewrite these tests using directly ShutdownMonitor without 200 lines of mock code that must to be bug-free and maintained.
I'd prefer to fix ShutdownMonitor than adding a mock for it.

@sbordet I have changed the final test testStopWait to talk to the real ShutdownMonitor. None of the other tests can use the real class because I need to test failure/fallback modes, and ShutdownMonitor is unsubclassable (private no-args constructor).

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyStopMojo.java

sbordet · 2022-01-14T12:31:50Z

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyStopMojo.java

+                            if (handle.isAlive())
+                            {
+                                handle = future.get(stopWait, TimeUnit.SECONDS);
+                            }


Don't think this block is correct.
future.get(wait, unit) throws TimeoutException if the time elapses, but the code seems to hint something like "if timeout and still alive, do something", so the "do something" will be skipped.

Have you considered using ProcessHandle.destroy[Forcibly]() if the timeout expires?
Perhaps along with CompletableFuture.orTimeout()?

@sbordet this would be a behaviour change of the JettyStopMojo: it has always just made a best effort to stop the remote jetty, waiting up to the configured time to get an acknowledgement from jetty that it is shutting down before giving up. I suppose we could add a new configuration option to forcibly destroy the process after the timeout expires, but I think that might be in another issue.

Signed-off-by: Jan Bartel <[email protected]>

sbordet · 2022-01-17T11:07:21Z

jetty-maven-plugin/src/test/java/org/eclipse/jetty/maven/plugin/MockShutdownMonitor.java

+ * org.eclipse.jetty.server.ShutdownMonitor.
+ *
+ */
+public class MockShutdownMonitor


I made ShutdownMonitor.start() and stop() public and I can rewrite these tests using directly ShutdownMonitor without 200 lines of mock code that must to be bug-free and maintained.
I'd prefer to fix ShutdownMonitor than adding a mock for it.

jetty-maven-plugin/src/test/java/org/eclipse/jetty/maven/plugin/TestJettyStopMojo.java

...y-maven-plugin/src/test/java/org/eclipse/jetty/maven/plugin/MockShutdownMonitorRunnable.java

jetty-maven-plugin/src/test/java/org/eclipse/jetty/maven/plugin/MockShutdownMonitor.java

...y-maven-plugin/src/test/java/org/eclipse/jetty/maven/plugin/MockShutdownMonitorRunnable.java

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyStopMojo.java

Signed-off-by: Jan Bartel <[email protected]>

…7344-wait-for-forked-jetty-process

janbartel · 2022-02-08T03:41:12Z

@sbordet latest release has gone out - can you now approve this pr?

sbordet

@janbartel a few suggested fixes have not been addressed (e.g. javadocs, semicolons, unnecessary types, etc.)?

* Issue #7344 Make plugin wait for forked jetty process to stop Signed-off-by: Jan Bartel <[email protected]> (cherry picked from commit 0b33877)

janbartel added 2 commits January 12, 2022 18:27

Issue #7344 Make plugin wait for forked jetty process to stop

42ce379

Signed-off-by: Jan Bartel <[email protected]>

Issue #7344 Updates

331d42a

Signed-off-by: Jan Bartel <[email protected]>

janbartel added the Enhancement label Jan 13, 2022

janbartel requested review from gregw, sbordet and lorban January 13, 2022 00:32

janbartel self-assigned this Jan 13, 2022

janbartel requested a review from olamy January 14, 2022 00:33

XakepSDK reviewed Jan 14, 2022

View reviewed changes

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyStopMojo.java Outdated Show resolved Hide resolved

sbordet requested changes Jan 14, 2022

View reviewed changes

Issue #7344 Update after review

6064805

Signed-off-by: Jan Bartel <[email protected]>

janbartel requested a review from sbordet January 15, 2022 00:07

sbordet requested changes Jan 17, 2022

View reviewed changes

Issue #7344 Changes post review

bfb988a

Signed-off-by: Jan Bartel <[email protected]>

janbartel requested a review from sbordet January 18, 2022 03:39

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

58eebc4

…7344-wait-for-forked-jetty-process

sbordet reviewed Feb 10, 2022

View reviewed changes

Issue #7344 Changes after review

774bcac

janbartel requested a review from sbordet February 10, 2022 22:24

sbordet approved these changes Feb 14, 2022

View reviewed changes

joakime changed the title ~~Jetty 10.0.x 7344 wait for forked jetty process~~ Issue #7344 - wait for forked jetty process Feb 14, 2022

olamy approved these changes Feb 16, 2022

View reviewed changes

janbartel merged commit 0b33877 into jetty-10.0.x Feb 21, 2022

janbartel deleted the jetty-10.0.x-7344-wait-for-forked-jetty-process branch February 21, 2022 12:46

joakime mentioned this pull request Feb 24, 2022

Catch up jetty-12.0.x with missing PRs #7652

Closed

22 tasks

joakime added the merge-12-needed label Feb 24, 2022

joakime added this to the 12.0.x milestone Feb 24, 2022

joakime pushed a commit that referenced this pull request Feb 25, 2022

Issue #7344 - wait for forked jetty process (#7374)

ec1f897

* Issue #7344 Make plugin wait for forked jetty process to stop Signed-off-by: Jan Bartel <[email protected]> (cherry picked from commit 0b33877)

joakime mentioned this pull request Oct 6, 2022

Confirm merge-12-needed labeled PRs have been merged #8690

Closed

51 tasks

joakime removed the merge-12-needed label Oct 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #7344 - wait for forked jetty process #7374

Issue #7344 - wait for forked jetty process #7374

janbartel commented Jan 13, 2022

sbordet Jan 14, 2022

janbartel Jan 14, 2022

sbordet Jan 17, 2022 •

edited

Loading

janbartel Jan 18, 2022

sbordet Jan 14, 2022

janbartel Jan 14, 2022

sbordet Jan 17, 2022

janbartel Jan 18, 2022

sbordet Jan 14, 2022

janbartel Jan 14, 2022 •

edited

Loading

sbordet Jan 17, 2022

janbartel commented Feb 8, 2022

sbordet left a comment

Issue #7344 - wait for forked jetty process #7374

Issue #7344 - wait for forked jetty process #7374

Conversation

janbartel commented Jan 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbordet Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janbartel Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janbartel commented Feb 8, 2022

sbordet left a comment

Choose a reason for hiding this comment

sbordet Jan 17, 2022 •

edited

Loading

janbartel Jan 14, 2022 •

edited

Loading