On Wed, Jun 24, 2020 at 2:53 AM Brian Candler <b.cand...@pobox.com> wrote:
>
> I have a kind-of workaround. Firstly, I see that Go has the ability to start
> a new session for the child with Setsid called. However on timeout I still
> need to kill the process group (-pid) instead of the process, which I can do
> by implementing the context deadline manually:
>
> cmd := exec.Command("/bin/sh", "-c", "exec yes >/dev/null")
> if cmd.SysProcAttr == nil {
> cmd.SysProcAttr = &syscall.SysProcAttr{}
> }
> cmd.SysProcAttr.Setsid = true
>
> go func() {
> select {
> case <-ctx.Done():
> // Race by doing it here and not in os/exec; maybe
> some other
> // process gets the pid in the mean time
> if cmd.Process.Pid > 1 && (cmd.ProcessState == nil ||
> !cmd.ProcessState.Exited()) {
> err := syscall.Kill(-cmd.Process.Pid,
> os.Kill.(syscall.Signal))
> if err != nil && err != syscall.ESRCH {
> fmt.Printf("Ooops, signal failed:
> %v\n", err)
> }
> }
> }
> }()
>
> err := cmd.Run()
> // rest of code the same
>
> This appears to solve both problems: I get sensible resource accounting,
> *and* the shell and its children are killed.
>
> 1 Run(): signal: killed
> rusage 1: Utime={0 271899}, Stime={0 727730}, Maxrss=1892
> 2 Run(): signal: killed
> rusage 2: Utime={0 459969}, Stime={1 539899}, Maxrss=2004
> 4 Run(): signal: killed
> rusage 4: Utime={0 979834}, Stime={3 19489}, Maxrss=1904
> Bye!
>
> To make this work without the race, I would like to suggest that the
> Process.Kill() function checks the value of SysProcAttr.Setsid, and if it's
> true, sends the kill signal to the process group rather than just the
> process. This way, the regular exec.CommandContext would clean up properly.
>
> (Note: I would *not* change Process.Signal(); any signal sent that way,
> including the Kill signal, should just go to the single process).
>
> This is a nominally a change in behaviour, although I can't see that the
> current behaviour is actually very useful - that is, killing the parent, but
> letting its children re-attach themselves to pid 1 as (in effect)
> unsupervised daemons. In addition, it would only be a change in behaviour
> for those people who have applied Setsid=true.
>
> However if necessary it could be made backwards-compatible by adding a new
> flag to SysProcAttr, e.g. "KillProcessGroup" which would make Process.Kill
> send to -pid instead of pid.
>
> Thoughts?
My concern is that once you get into this area, you are dealing with
issues that are Unix-specific. The os package tries to be roughly
OS-independent. Perhaps it was a mistake to add os.Process.Kill and
os.Process.Signal at all, but we can't remove them now. But for
adding new mechanisms, I think it's time to use golang.org/x/sys/unix,
in this case unix.Kill with a negative number to send the signal to
the process group.
Ian
--
You received this message because you are subscribed to the Google Groups
"golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to golang-nuts+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/golang-nuts/CAOyqgcXLuX9PjOb090iyDxKD2do4qY4OHJAZpQz5-qeCgvyMwA%40mail.gmail.com.
