lbartnik / subprocess

Other
49 stars 10 forks source link

Terminating subprocess instantiated children #1

Closed johndharrison closed 7 years ago

johndharrison commented 7 years ago

Hi Łukasz,

This package looks very useful. When we kill a process does subprocess handle the termination of any children the subprocess may have spawned:

proc1 <- tempfile(fileext = ".sh")
proc2 <- tempfile(fileext = ".sh")
cat("#!/bin/sh\n", file = proc1)
cat("#!/bin/sh\n", file = proc2)
cat("sh ", proc2, "\n", sep = "", file = proc1, append = TRUE)
cat("sleep 120\n", file = proc2, append = TRUE)
Sys.chmod(proc1, "700")
Sys.chmod(proc2, "700")

handle1 <- spawn_process(proc1)
process_kill(handle1)
lbartnik commented 7 years ago

Hi,

Yes, if you call process_kill() or process_terminate(), waitpid() or GetExitCodeProcess() are called internally. However, if you call process_send_signal(), then you have to explicitly call process_poll() (which should be probably called process_wait()).

By the way, this is not on CRAN yet, how did you find it?

Lukasz

johndharrison commented 7 years ago

Hi Lukasz,

I saw your post on SO with regards to process management. My point above is that currently from what I can see child processes that the sub process spawned are not being terminated when the sub process itself is terminated. So in the example above proc1 is terminated but proc2 continues running.

lbartnik commented 7 years ago

Oh, I see... well, in Linux I'd send the signal to a group of processes, if the first child created one. I don't know if there is a way to kill all descendants in of a given child if the child didn't create a process group. I also don't know how to approach this on Windows - I'm much more familiar with Linux than WINAPI.

You can also use pskill() (which I'd like to adapt in subprocess at some point) to send the kill signal to the child and its children.

Finally, I'd say it's the child's responsibility to handle it's own children. But maybe we could create a new process group (in Linux) for each child and then send the signal to the whole group.

What do you think? And what is your scenario? I'd like this package to address a broad range of use-cases - I'm wondering if what you want doesn't interfere with what other people might want to do.

And thanks for the feedback, that's exactly what I hoped for!

johndharrison commented 7 years ago

Yes a process group should work or a session id (http://man7.org/linux/man-pages/man2/setsid.2.html). See here for discussion re python subprocess http://stackoverflow.com/questions/4789837/how-to-terminate-a-python-subprocess-launched-with-shell-true . Windows from what little I know has a process group flag that can be set.

This situation would arise often for example nodejs modules and JAVA jvm jar files. I think the danger is that the user wouldn't realise that the sub process is spawning children and then they are left with orphaned processes when they terminate the parent subprocess.

lbartnik commented 7 years ago

I agree, this is a valid point. I think creating and killing the whole group should be the default, with an option to handle only the immediate child.

I'm going to improve in this regard but I want to get the first version to CRAN first and see what other comments people might have. Thanks for letting me know!

lbartnik commented 7 years ago

I think I got this fixed in Windows: https://github.com/lbartnik/subprocess/tree/terminate-group Linux will be a piece of cake now.

johndharrison commented 7 years ago

Yeah that looks to be killing the child process in windows.

proc1 <- tempfile(fileext = ".bat")
proc2 <- tempfile(fileext = ".bat")
cat("cmd /c", proc2, "\n", sep = "", file = proc1)
cat("ping -n 120 127.0.0.1 >nul", file = proc2)
Sys.chmod(proc1, "700")
Sys.chmod(proc2, "700")
handle1 <- spawn_process(proc1)
lbartnik commented 7 years ago

Good! Thank you dot confirming that. I'm going to fix the Linux part and then finally submit it to cran

lbartnik commented 7 years ago

Hey @johndharrison, I've just pushed the Linux part of changes to https://github.com/lbartnik/subprocess/tree/terminate-group. I'm a bit confused with the results of tests - could you please verify that it solves your original Linux scenario?

johndharrison commented 7 years ago

Hi @lbartnik yeah the terminate-group branch is solving the original problem I was encountering on linux.

lbartnik commented 7 years ago

solved in #6