classification
Title: subprocess.check_output with shell=True ignores the timeout
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.7
process
Status: closed Resolution: duplicate
Dependencies: 5115 Superseder: subprocess.run timeout does not function if shell=True and capture_output=True
View: 37424
Assigned To: Nosy List: Daniel Shaulov, astrand, fangyizhou, haizaar, martin.panter
Priority: normal Keywords: patch

Created on 2016-03-10 23:45 by Daniel Shaulov, last changed 2019-07-01 00:06 by gregory.p.smith. This issue is now closed.

Files
File name Uploaded Description Edit
killpg.patch Daniel Shaulov, 2016-03-10 23:45 Adds killpg method and kill_group argument review
Messages (3)
msg261533 - (view) Author: Daniel Shaulov (Daniel Shaulov) * Date: 2016-03-10 23:45
Basically -
subprocess.check_output("ping localhost", shell=True, timeout=5)
Will hang forever.

What happens is that subprocess.run times out on communicate, sends SIGKILL *to the shell* and then calls communicate again without the timeout. But nothing actually closes the "ping" command so the second communicate will never exit.

Even if we put a timeout on the second communicate, it won't be good enough because that "ping" will still be "leaked".

This SO question is about the fact that kill doesn't work when shell=True, and might be relevant for this issue:
http://stackoverflow.com/questions/4789837/how-to-terminate-a-python-subprocess-launched-with-shell-true

As a possible fix I propose doing the following:
1. Add killpg to the Popen class.
2. Add an argument to run - "kill_group" that makes run use killpg instead of kill
3. Users can all:
subprocess.check_output("ping localhost", shell=True, start_new_session=True, kill_group=True, timeout=5)

And have it work fine. This is the best I could come up with, without breaking any existing behavior.

Other options to consider:
1. Not have kill_group argument and to implicitly kill by group when start_new_session is True (but this is not really backwards compatible).
2. Having kill_group as an argument for Popen and not run, and when kill is called, it will call killpg if the option was specified.

A patch is attached with my proposed solution.
msg292221 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2017-04-24 11:08
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?
msg292225 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2017-04-24 11:47
Issue 5115 is already open with patch that has an alternative API to the low-level “killpg” method. It also has a Windows implementation and tests. I suggest to focus this bug on the higher-level “kill_group” option.
History
Date User Action Args
2019-07-01 00:06:43gregory.p.smithsetstatus: open -> closed
superseder: subprocess.run timeout does not function if shell=True and capture_output=True
resolution: duplicate
stage: patch review -> resolved
2019-06-27 13:11:54haizaarsetnosy: + haizaar
2018-02-16 13:03:48fangyizhousetnosy: + fangyizhou
2017-04-24 11:47:07martin.pantersetdependencies: + Extend subprocess.kill to be able to kill process groups
messages: + msg292225
2017-04-24 11:08:46martin.pantersetversions: + Python 3.7, - Python 3.5, Python 3.6
nosy: + martin.panter

messages: + msg292221

type: behavior -> enhancement
stage: patch review
2016-03-10 23:45:31Daniel Shaulovcreate