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

[serve] Gracefully shut down on SIGTERM #17325

Merged
merged 1 commit into from Jun 18, 2019

Conversation

jugglinmike
Copy link
Contributor

No description provided.

@Hexcles
Copy link
Member

Hexcles commented Jun 13, 2019

I don't think this always works -- it probably only works in an interactive shell when you press Ctrl-D or Ctrl-C.

You're hoping that the subprocesses will receive the signal, so you can simply ignore the signal in the parent process and wait for them to exit. However, it is the shell that sends the signal recursively to all foreground jobs. If you use kill to send a SIGINT/SIGTERM directly to the main, parent process, the kernel won't automatically propagate that down, so the parent process will wait for the subprocesses forever.

The proper way to do this, I think, is to:

  1. In the parent process, before starting any subprocesses, install a no-op signal handler to be inherited by the subprocesses, so that the subprocesses won't exit themselves when Ctrl-C/Ctrl-D is pressed.
  2. The parent process, after starting all subprocesses, installs a new signal handler that will set a flag and let the parent process terminate/kill subprocesses upon receiving SIGINT/SIGTERM.

This will make sure the behaviour is the same regardless of whether you press Ctrl-C/D in a terminal or send a SIGINT/SIGTERM to the main process.

One caveat is that I'm not sure step 1 also works on Windows (subprocesses/threads aren't created by forking/cloning on Windows, and in fact I don't know about signal handling in general on Windows). Step 1 also might not be strictly necessary. Without step 1, the parent process should attempt to terminate and kill the subprocesses, or join them if they are already dead.

@jugglinmike
Copy link
Contributor Author

This has worked during my functional testing--sending via Ctrl-C on a terminal and also using kill, all on GNU/Linux. That doesn't mean it works everywhere, of course.

I assumed that all of the servers were running in daemonized threads, but I don't actually know what happens to a process created from such a thread.

Whatever the case, this was intended to be a simple change to make wptserve behave a little more idiomatically. (I'm doing some scripting with wptserve at the moment.) I don't think that warrants a lot of additional complexity, and I'm just as happy to send a SIGINT event to stop the process. If you and @jgraham agree, then I'll close this.

@Hexcles
Copy link
Member

Hexcles commented Jun 14, 2019

I assumed that all of the servers were running in daemonized threads, but I don't actually know what happens to a process created from such a thread.

Aha! This is actually the important difference! The daemon subprocesses (not threads; threads are not used in this module) will be cleaned up when the parent exits (https://docs.python.org/2/library/multiprocessing.html#multiprocessing.Process.daemon).

If you look at the logs, you'll see that the subprocesses aren't terminated until the parent process itself exits.

Copy link
Member

@Hexcles Hexcles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words, this LGTM!

@gsnedders gsnedders merged commit 2fdf0cb into web-platform-tests:master Jun 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants