Close all remaining children on exit#652
Conversation
| function close(child: ChildProcess): Promise<void> { | ||
| const onclose = new Promise<void>((resolve) => { | ||
| child.on('exit', (code, signal) => { | ||
| console.warn(`pid=${child.pid} exited with code=${code} and signal=${signal}`); | ||
| resolve(); | ||
| }); | ||
| }); | ||
|
|
||
| child.kill('SIGTERM'); | ||
| child.kill('SIGINT'); | ||
| child.kill('SIGKILL'); | ||
| return onclose; | ||
| } |
There was a problem hiding this comment.
Please tell me your thoughts on sending this barrage of signals
There was a problem hiding this comment.
Not as a familiar with this but would we not want to chain these calls without some check in case the PID is reassigned?
https://nodejs.org/api/child_process.html#child_process_subprocess_kill_signal
The ChildProcess object may emit an 'error' event if the signal cannot be delivered. Sending a signal to a child process that has already exited is not an error but may have unforeseen consequences. Specifically, if the process identifier (PID) has been reassigned to another process, the signal will be delivered to that process instead which can have unexpected results.
I don't know if there is a good solution to this. At most you can check .kill() return value to make sure it received the signal. But that I still think isn't an indication that the process was actually killed, just that it received the signal. It is at least a secondary check to prevent getting to the weird reassignment PID state.
There was a problem hiding this comment.
I'm a bit slow, I didn't realize that the exit event had the signal if the process terminated using a signal. You could change this so it uses some sort of cascading timeout to elevate the kill signal and reduce the chance calling SIGKILL on a PID reassignment. So instead of blasting the signals it elevates and gives it time to clean up if needed. Just give some time between SIGTERM/SIGINT and SIGKILL for the process to hopefully properly terminate and then finally kill it if it goes beyond some timeout (I don't know what that should be, 30000ms?). No clue if any of this makes any sense or if there is a better way to do this.
There was a problem hiding this comment.
I cannot, unfortunately. Any use of the event scheduler (such as with callbacks or async api) will be dropped. This does mean that the exit event will often not be caught and you just have to trust that the process exited.
BryonLewis
left a comment
BryonLewis
left a comment
There was a problem hiding this comment.
Did a run through on Windows as well. It was confirmed to be killing the outstanding processes on a force exit while pipelines were running.
2 participants
fixes #597