bpo-18299: extending a kill_on_os_error() in Popen by shihai1991 · Pull Request #13908 · python/cpython (original) (raw)

I'm rejecting this PR. This is not an API we need on the subprocess.Popen class.

There may be utility on a context manager that tries to terminate a process when an exception is escaping the context, but that belongs in its own library. It isn't something users of subprocess need on a regular basis.

The PR references bpo-18299 but I see no relation to that bug. If something like this would've been used in that bug maybe this belongs in test.script_helper.

Misc review notes for posterity here rather than trying to connect them with parts of the diffs:

The description in the documentation still makes no sense. We don't protect anyone from race conditions or hangs. And the method name is still misleading. It appears to attempt to kill the process if any exception escapes the context manager followed by waiting for it regardless of kill succeeding or not (that wait may hang if not). Yet the method name is confusing as a call to Popen.kill() tries to kill the process immediately while a call to Popen.kill_on_os_error() does nothing unless the caller uses it as a context manager. A better name if this were to be a method at all would be something more along the lines of .get_killing_context.

It is possible to write unittests for this functionality, tests would be required.