WordPress / playground-tools

GNU General Public License v2.0
127 stars 38 forks source link

wp-now: Improve job control *or* support daemon (background) mode #220

Open mcsf opened 2 months ago

mcsf commented 2 months ago

The problem

In typical usage, a user invokes wp-now in an interactive shell session, monitors the server's log messages in that same session, then terminates via ctrl-c.

Programs of this kind often offer an option to run in the background — often described as running as a daemon. This is useful for many reasons, and in this mode programs typically log messages directly to a file rather than stdout/stderr, giving users the freedom to read them after the fact or in realtime (e.g. tail -f $LOGFILE).

Coping using background processes

Currently, in the absence of a daemon mode, a user might reasonably expect to achieve a similar result by sending the wp-now process to the background. Hypothetically:

# Redirect output to log file and send process to the background
$ wp-now start > my-log 2>&1 &

# Save the process ID for later
$ WPNOW_PID=$!

sleep 10

# Then kill the server
$ kill $WPNOW_PID

For single-process servers, this works. But in the case of wp-now, this won't kill the server. It will merely kill the original process that was started by invoking wp-now start. The server itself will remain as an orphan process. See this process tree:

       \-+= -sh
         \-+= wp-now start
           \-+- node {trimmed}/.../bin/wp-now start
             \--- node {trimmed}/.../wp-now/cli.js start

The actual server is the leaf process. Killing any of its ancestors doesn't stop it.

A note on process groups

Then why does ctrl-c terminate the server in a typical interactive session? Well, TIL that ctrl-c causes most TTY devices to send SIGINT not just to the foreground process, but to the foreground process group. This can be emulated programmatically with kill with the following "negative" form:

wp-now start & PID=$!
kill -- -$PID  # Success!

When we invoke wp-now start &, the outermost process becomes the leader of a new process group, and its two descendants (see tree above) belong to that group. So this special form of kill effectively terminates everything, as intended.

However, this doesn't always work out of the box, e.g. in non-interactive sessions. My use case is a git-bisect script that fetches-or-builds Gutenberg for the current revision and instantiates a new wp-now site and terminates it after the user has tested the site. Here, the process group is led by the outermost Git process (git bisect run my-script.sh), so I can't rely on that, and I need (to the best of my knowledge) to manually collect wp-now's descendants processes.

Proposals

For my purposes, either one of the following would be enough. But technically they are complementary: even in the presence of a daemon mode, we could improve how processes are created in wp-now.

Alternative 1: Add a -d switch

Implement a true daemon mode for wp-now:

$ wp-now start -d
Server running at http://localhost:8881. Log files at /tmp/foobar.

$ wp-now stop
Server stopped at http://localhost:8881. Log files at /tmp/foobar.

Alternative 2: Streamline the process cascade

I don't have any knowledge of wp-now's architecture, so this is all speculative. But a few things could be explored:

Attachment: an example with exec

# script-a.sh
echo "Hello from script A. My PID is $$"
exec sh script-b.sh
echo "This line will never run because we have replaced ourselves with 'sh script-b.sh'"

# script-b.sh
echo "Hello from script B. My PID is $$"

Running sh script-a.sh will output:

Hello from script A. My PID is 1234.
Hello from script B. My PID is 1234. # <-- same PID
sejas commented 2 months ago

Thank you for all the details @mcsf. I think the proposal makes a lot of sense and will help to unlock some use cases for executing Playground inside GitHub actions.

flexseth commented 2 months ago

Another aspect about job control - currently there's no easy way to see what Playgrounds are running

A CLI command to see what wp-now is doing might be helpful, so you're not running three WordPress servers all the time for a week straight (without knowing it) :)

adamziel commented 2 months ago

I had to do ps aux | grep wp-now | awk '{print $2}' | xargs kill -9 to be able to work on the static site generator.

flexseth commented 2 months ago

surfacing / related

mcsf commented 2 months ago

I had to do ps aux | grep wp-now | awk '{print $2}' | xargs kill -9 to be able to work on the static site generator.

Just as a little tip until we have some improvement: you could simply do pkill -9 -f wp-now. :)

adamziel commented 2 months ago

This could be a good start towards a solution:

diff --git a/packages/wp-now/public/with-node-version.js b/packages/wp-now/public/with-node-version.js
index 9268c249..7dc479d9 100644
--- a/packages/wp-now/public/with-node-version.js
+++ b/packages/wp-now/public/with-node-version.js
@@ -49,6 +49,20 @@ if (!meetsMinimumVersion(minimum, [major, minor, patch])) {

 // Launch the wp-now process and pipe output through this wrappers streams.
 const dir = path.dirname(fs.realpathSync(process.argv[1]));
-child_process.spawn('node', [dir + '/cli.js', ...process.argv.slice(2)], {
+const child = child_process.spawn('node', [dir + '/cli.js', ...process.argv.slice(2)], {
    stdio: ['inherit', 'inherit', 'inherit'],
 });
+
+child.on('exit', (code) => {
+   // Clean up any resources here
+   console.log('Child process exited with code', code);
+   // Perform any necessary cleanup operations before exiting
+   // ...
+});
+
+process.on('exit', () => {
+   // Clean up any resources here
+   console.log('Exiting...');
+   // Perform any necessary cleanup operations before exiting
+   // ...
+});