Author martin.panter
Recipients Daniel Shaulov, astrand, martin.panter
Date 2017-04-24.11:08:45
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1493032126.06.0.896039757216.issue26534@psf.upfronthosting.co.za>
In-reply-to
Content
I don’t know enough about process groups and sessions to properly review, but some things that stand out:

* Patch is missing documentation and tests
* If the “killpg” just wraps os.killpg, perhaps adding the method is not justified
* Are there any race conditions with killing a process group that has already exited. When does a process group get freed and potentially reused (so you may kill the wrong group)? The “send_signal” method (and others) have a check to avoid signalling an unrelated process.
* The method is called killpg, and the doc string mentions SIGKILL, but the implementation says SIGTERM
* What happens if you use killpg without starting a new session? If it kills the parent process as well, that sounds like a source of subtle bugs that may only be detected in unexpected cases (e.g. Ctrl+C or timeout)
* Be aware of Issue 25942. It is not clear what should happen to the child process(es) when the timeout happens, or when the “communicate” call is interrupted.
* What platforms does this support and what happens if there is no platform support?
History
Date User Action Args
2017-04-24 11:08:46martin.pantersetrecipients: + martin.panter, astrand, Daniel Shaulov
2017-04-24 11:08:46martin.pantersetmessageid: <1493032126.06.0.896039757216.issue26534@psf.upfronthosting.co.za>
2017-04-24 11:08:46martin.panterlinkissue26534 messages
2017-04-24 11:08:45martin.pantercreate