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

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

Open
mcsf opened this issue Apr 5, 2024 · 6 comments
Open

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

mcsf opened this issue Apr 5, 2024 · 6 comments
Labels
Enhancement New feature or request VS Code wp-now

Comments

@mcsf
Copy link

mcsf commented Apr 5, 2024

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:

  • Understand why three processes must run instead of one. What are the two outermost processes doing? I suppose they are doing things like parsing command options and delegating with something like Node's child_process.spawn()? In POSIX shell and in other programming languages, we can readily use the system's execve API, which replaces the current process with the thing that it is calling, thereby preserving the original process ID (PID). It seems like this isn't available via Node.js's APIs but can be achieved with native addons.
  • If that can't be achieved, consider setting a trap in the outermost process so that, if it is to be terminated, it will kill its descendants before it dies.

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
@mcsf mcsf added the Enhancement New feature or request label Apr 5, 2024
@sejas
Copy link
Collaborator

sejas commented Apr 5, 2024

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
Copy link

flexseth commented Apr 5, 2024

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
Copy link
Collaborator

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
Copy link

flexseth commented Apr 17, 2024

@mcsf
Copy link
Author

mcsf commented Apr 18, 2024

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 kill -9 $(pgrep -f wp-now). :) (It's important to not use double quotes around the command substitution.)

@adamziel
Copy link
Collaborator

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
+	// ...
+});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request VS Code wp-now
Projects
None yet
Development

No branches or pull requests

4 participants