feat(cmdutil): TerminateProcessGroup for graceful termination #13792
Conversation
Changelog[uncommitted] (2023-08-26)Miscellaneous
|
Adds a new **currently unused** function TerminateProcessGroup that terminates all processes in a group gracefully. It does so by first sending the process a SIGINT on Unix systems, and CTRL_BREAK_EVENT on Windows, and waiting a specified duration for the process to exit. The choice of signals was very deliberate and is documented in the comments for TerminateProcessGroup. If the process does not exit in the given duration, it and its child processes are forcibly terminated with SIGKILL or equivalent. Testing: The core behaviors are tested against Python, Go, and Node. The corner cases of signal handling are tested against rogue Go processes. The changes were experimented with in #13760. Refs #9780
abhinav
force-pushed
the
abhinav/cmdutil-terminate
branch
from
82ded8b
to
cb8a955
Compare
Uses the new TerminateProcessGroup functionality introduced in #13792 to shut down plugins gracefully. Graceful shutdown takes the following form: - Send a termination signal (SIGINT or CTRL_BREAK_EVENT) - Wait up to 1 second for the plugin to exit - Kill it with SIGKILL Note that TerminateProcessGroup kills the entire group so we don't need a separate KillChildren and cmd.Process.Kill(). This change also deprecates cmdutil.KillChildren since we shouldn't really be using SIGKILL as a first resort anyway. Resolves #9780
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
Uses the new TerminateProcessGroup functionality introduced in #13792 to shut down plugins gracefully. Graceful shutdown takes the following form: - Send a termination signal (SIGINT or CTRL_BREAK_EVENT) - Wait up to 1 second for the plugin to exit - Kill it with SIGKILL Note that TerminateProcessGroup kills the entire group so we don't need a separate KillChildren and cmd.Process.Kill(). This change also deprecates cmdutil.KillChildren since we shouldn't really be using SIGKILL as a first resort anyway. Resolves #9780
| return err | ||
| } | ||
|
|
||
| // Kill the root process since KillChildren only kills child processes. |
There was a problem hiding this comment.
Why does windows kill the root process but unix doesn't?
There was a problem hiding this comment.
It does. The killProcessGroup for Unix calls KillChildren, which sends the signal to the entire group, which includes the root.
(Signal to -pid goes to the entire group.)
We have to do it manually on Windows because the TerminateProcess syscall (which is what os.Process.Kill uses) doesn't go to the whole group. The implementation of KillChildren for Windows only killed the child processes. So there was a discrepancy in Windows vs Unix behaviors of our KillChildren implementations. I didn't want to change that behavior since KillChildren is public, so I added killProcessGroup, which also kills the root process explicitly.
There was a problem hiding this comment.
So KillChildren(pid) on windows is just kill the children, but KillChildren on unix is kill children and self?
That's upsetting.
There was a problem hiding this comment.
Yeah. It hasn't been a problem because the only place the function is used, the root process is killed afterwards anyway.
There was a problem hiding this comment.
Can we just remove the KillChildren function inline it's parts into KillProcessGroup and just have that?
There was a problem hiding this comment.
I didn't want to break it in case someone was using it directly. Next PR deprecates it, though.
I'm open to deleting it if we're not concerned about breaking it but it probably makes sense to do that in the same PR where we stop using it.
There was a problem hiding this comment.
I'm not concerned, sdk/go/common isn't exactly a stable public api and most of it is only used by pkg.
But if the next PR removes the use of it then I'm fine with this change as is, and we'll remove it in that one.
Uses the new TerminateProcessGroup functionality introduced in #13792 to shut down plugins gracefully. Graceful shutdown takes the following form: - Send a termination signal (SIGINT or CTRL_BREAK_EVENT) - Wait up to 1 second for the plugin to exit - Kill it with SIGKILL Note that TerminateProcessGroup kills the entire group so we don't need a separate KillChildren and cmd.Process.Kill(). This change also deprecates cmdutil.KillChildren since we shouldn't really be using SIGKILL as a first resort anyway. Resolves #9780
Uses the new TerminateProcessGroup functionality introduced in #13792 to shut down plugins gracefully. Graceful shutdown takes the following form: - Send a termination signal (SIGINT or CTRL_BREAK_EVENT) - Wait up to 1 second for the plugin to exit - Kill it with SIGKILL Note that TerminateProcessGroup kills the entire group so we don't need a separate KillChildren and cmd.Process.Kill(). This change also deprecates cmdutil.KillChildren since we shouldn't really be using SIGKILL as a first resort anyway. Note that this does not modify the behavior of individual plugins. Those will exit as usual but with a SIGINT instead of SIGKILL terminating them. Resolves #9780
Uses the new TerminateProcessGroup functionality introduced in #13792 to shut down plugins gracefully. Graceful shutdown takes the following form: - Send a termination signal (SIGINT or CTRL_BREAK_EVENT) - Wait up to 1 second for the plugin to exit - Kill it with SIGKILL Note that TerminateProcessGroup kills the entire group so we don't need a separate KillChildren and cmd.Process.Kill(). This change also deprecates cmdutil.KillChildren since we shouldn't really be using SIGKILL as a first resort anyway. Note that this does not modify the behavior of individual plugins. Those will exit as usual but with a SIGINT instead of SIGKILL terminating them. Resolves #9780
Adds a new currently unused function TerminateProcessGroup
that terminates all processes in a group gracefully.
It does so by first sending the process
a SIGINT on Unix systems, and CTRL_BREAK_EVENT on Windows,
and waiting a specified duration for the process to exit.
The choice of signals was very deliberate
and is documented in the comments for TerminateProcessGroup.
If the process does not exit in the given duration,
it and its child processes are forcibly terminated
with SIGKILL or equivalent.
Testing:
The core behaviors are tested against Python, Go, and Node.
The corner cases of signal handling are tested
against rogue Go processes.
The changes were experimented with in #13760.
Refs #9780