Message292221
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? |
|
Date |
User |
Action |
Args |
2017-04-24 11:08:46 | martin.panter | set | recipients:
+ martin.panter, astrand, Daniel Shaulov |
2017-04-24 11:08:46 | martin.panter | set | messageid: <1493032126.06.0.896039757216.issue26534@psf.upfronthosting.co.za> |
2017-04-24 11:08:46 | martin.panter | link | issue26534 messages |
2017-04-24 11:08:45 | martin.panter | create | |
|