search

rtomayko / posix-spawn

Ruby process spawning library
Other
520 stars 52 forks source link

POSIX::Spawn::Child sends TERM to whole process group on timeout / error #68

Closed rtomayko closed 9 years ago

rtomayko commented 9 years ago

This adds a :pgroup_kill option to POSIX::Spawn::Child. When both :pgroup => true and :pgroup_kill => true are set, an exception like TimeoutExceeded or MaximumOutputExceeded will cause SIGTERM to be sent to the whole child process group instead of just the immediate child process.

Need tests here and I'm still not sure I love this combination of args just yet. I think we should raise ArgumentError if :pgroup_kill => true and :pgroup is unset or set to something other than true at least.

/cc @github/gitrpc, @piki, @adelcambre, https://github.com/github/gitrpc/pull/369, https://github.com/github/ops/issues/4263

adelcambre commented 9 years ago

Definite :+1: on ArgumentError or something. Killing yourself is a pretty nasty failure mode for not doing the right thing.

Reading the code now, this makes me think maybe just killing the group when pgroup is passed would be fine. If it's an error to pass :kill_pgroup without :pgroup, would there ever be a case you'd want to pass :pgroup without :kill_pgroup? If neither case is desired, one option seems fine.

adelcambre commented 9 years ago

Heh, just realized I used :kill_pgroup rather than :pgroup_kill, the former reads better to me but I'm not worried either way.

rtomayko commented 9 years ago

Added some basic stuff to the tests for checking that child processes were reaped but they're not checking for anything below the immediate child. Need to figure something out for that, like listing processes by pgid maybe.

rtomayko commented 9 years ago

As for the option, I'm going to leave it separate from :pgroup but :pgroup_kill => true will imply :pgroup => true. So you can set :pgroup without enabling group signalling but you can't enable group signaling without enabling :pgroup.

rtomayko commented 9 years ago

Lastly, I think :pgroup_kill => true should be the default for POSIX::Spawn::Child in a future major version rev. The Child abstraction should have good defaults for process groups. It's surprising that Child can return while processes it spawned are still running. I don't feel that way about the primitive #spawn, #system, #popen calls but none of those manage killing the processes they spawn.