Issue #7344 - wait for forked jetty process #7374
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| // | ||
| // ======================================================================== | ||
| // Copyright (c) 1995-2022 Mort Bay Consulting Pty Ltd and others. | ||
| // | ||
| // This program and the accompanying materials are made available under the | ||
| // terms of the Eclipse Public License v. 2.0 which is available at | ||
| // https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 | ||
| // which is available at https://www.apache.org/licenses/LICENSE-2.0. | ||
| // | ||
| // SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 | ||
| // ======================================================================== | ||
| // | ||
|
|
||
| package org.eclipse.jetty.maven.plugin; | ||
|
|
||
| import java.net.ServerSocket; | ||
|
|
||
| import org.eclipse.jetty.toolchain.test.IO; | ||
|
|
||
| /** | ||
| * MockShutdownMonitor | ||
janbartel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * A helper class that grabs a ServerSocket, spawns a thread and then | ||
| * passes the ServerSocket to the Runnable. This class has a main so | ||
| * that it can be used for forking, to mimic the actions of the | ||
| * org.eclipse.jetty.server.ShutdownMonitor. | ||
| */ | ||
| public class MockShutdownMonitor | ||
|
There was a problem hiding this comment. Why can't we use There was a problem hiding this comment. @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. There was a problem hiding this comment. I made There was a problem hiding this comment. @sbordet I have changed the final test |
||
| { | ||
| String key; | ||
| MockShutdownMonitorRunnable testerRunnable; | ||
| ServerSocket serverSocket; | ||
|
|
||
| public MockShutdownMonitor(String key, MockShutdownMonitorRunnable testerRunnable) | ||
| throws Exception | ||
| { | ||
| this.key = key; | ||
| this.testerRunnable = testerRunnable; | ||
| listen(); | ||
| } | ||
|
|
||
| private ServerSocket listen() | ||
| throws Exception | ||
| { | ||
| serverSocket = new ServerSocket(0); | ||
| try | ||
| { | ||
| serverSocket.setReuseAddress(true); | ||
| return serverSocket; | ||
| } | ||
| catch (Throwable e) | ||
| { | ||
| IO.close(serverSocket); | ||
| throw e; | ||
| } | ||
| } | ||
|
|
||
| public int getPort() | ||
| { | ||
| if (serverSocket == null) | ||
| return 0; | ||
| return serverSocket.getLocalPort(); | ||
| } | ||
|
|
||
| public void start() | ||
| throws Exception | ||
| { | ||
| testerRunnable.setServerSocket(serverSocket); | ||
| testerRunnable.setKey(key); | ||
| Thread thread = new Thread(testerRunnable); | ||
| thread.setDaemon(true); | ||
| thread.setName("Tester Thread"); | ||
| thread.start(); | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| // | ||
| // ======================================================================== | ||
| // Copyright (c) 1995-2022 Mort Bay Consulting Pty Ltd and others. | ||
| // | ||
| // This program and the accompanying materials are made available under the | ||
| // terms of the Eclipse Public License v. 2.0 which is available at | ||
| // https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 | ||
| // which is available at https://www.apache.org/licenses/LICENSE-2.0. | ||
| // | ||
| // SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 | ||
| // ======================================================================== | ||
| // | ||
|
|
||
| package org.eclipse.jetty.maven.plugin; | ||
|
|
||
| import java.io.InputStreamReader; | ||
| import java.io.LineNumberReader; | ||
| import java.io.OutputStream; | ||
| import java.net.ServerSocket; | ||
| import java.net.Socket; | ||
| import java.nio.charset.StandardCharsets; | ||
|
|
||
| import org.eclipse.jetty.toolchain.test.IO; | ||
|
|
||
| /** | ||
| * MockShutdownMonitorRunnable | ||
janbartel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * | ||
| * Mimics the actions of the org.eclipse.jetty.server.ShutdownMonitor.ShutdownMonitorRunnable | ||
| * to aid testing. | ||
| */ | ||
| public class MockShutdownMonitorRunnable implements Runnable | ||
| { | ||
| ServerSocket serverSocket; | ||
| String key; | ||
| String statusResponse = "OK"; | ||
| String pidResponse; | ||
| String defaultResponse = "Stopped"; | ||
| boolean exit; | ||
|
|
||
| public void setExit(boolean exit) | ||
| { | ||
| this.exit = exit; | ||
| } | ||
|
|
||
| public void setKey(String key) | ||
| { | ||
| this.key = key; | ||
| } | ||
|
|
||
| public void setServerSocket(ServerSocket serverSocket) | ||
| { | ||
| this.serverSocket = serverSocket; | ||
| } | ||
|
|
||
| public void setPidResponse(String pidResponse) | ||
| { | ||
| this.pidResponse = pidResponse; | ||
| } | ||
|
|
||
| public void run() | ||
| { | ||
| try | ||
| { | ||
| while (true) | ||
| { | ||
| try (Socket socket = serverSocket.accept()) | ||
| { | ||
| LineNumberReader reader = new LineNumberReader(new InputStreamReader(socket.getInputStream())); | ||
| String receivedKey = reader.readLine(); | ||
| if (!key.equals(receivedKey)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| String cmd = reader.readLine(); | ||
| OutputStream out = socket.getOutputStream(); | ||
|
|
||
| if ("status".equalsIgnoreCase(cmd)) | ||
| { | ||
| out.write((statusResponse + "\r\n").getBytes(StandardCharsets.UTF_8)); | ||
| out.flush(); | ||
| } | ||
| else if ("pid".equalsIgnoreCase(cmd)) | ||
| { | ||
| out.write((pidResponse + "\r\n").getBytes(StandardCharsets.UTF_8)); | ||
| out.flush(); | ||
| } | ||
| else | ||
| { | ||
| out.write((defaultResponse + "\r\n").getBytes(StandardCharsets.UTF_8)); | ||
| out.flush(); | ||
| if (exit) | ||
| System.exit(0); | ||
| } | ||
| } | ||
| catch (Throwable x) | ||
| { | ||
| x.printStackTrace(); | ||
| } | ||
| } | ||
| } | ||
| catch (Throwable x) | ||
| { | ||
| x.printStackTrace(); | ||
| } | ||
| finally | ||
| { | ||
| IO.close(serverSocket); | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the sender is interested in getting confirmation from the remote jetty, then they configure a non-zero
stopWaittimeout, 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 tostopWaitseconds 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
ShutdownMonitoris to stop jetty before replying with "Stopped" and exiting. In the case where the caller has not configured astopWaittime they do not want to wait until the socket is closed etc.