classification
Title: `subprocess.run` documentation doesn't tell is using `stdout=PIPE` safe
Type: Stage: commit review
Components: Documentation Versions: Python 3.9, Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: docs@python Nosy List: bbayles, docs@python, gregory.p.smith, josh.r, martin.panter, pekka.klarck
Priority: normal Keywords: 3.7regression, patch

Created on 2018-04-20 13:05 by pekka.klarck, last changed 2019-03-23 07:51 by gregory.p.smith. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 12508 merged gregory.p.smith, 2019-03-23 07:25
PR 12509 merged miss-islington, 2019-03-23 07:40
Messages (6)
msg315510 - (view) Author: Pekka Klärck (pekka.klarck) Date: 2018-04-20 13:05
I'm porting old scripts from Python 2.7 to 3.6 and plan to change `subprocess.call()` to `subprocess.run()` at the same time. When using `call()` I've used `tempfile.TemporaryFile` as stdout because it's documentation has this warning:

    Note: Do not use stdout=PIPE or stderr=PIPE with this function. The child process will block if it generates enough output to a pipe to fill up the OS pipe buffer as the pipes are not being read from.

Interestingly there is no such note in the docs of `run()`, and based on my (possibly inadequate) testing I couldn't get it to hang either. I'm still somewhat worried about using `stdout=PIPE` with it because the docs don't explicitly say it would be safe. I'm especially worried because the docs of `call()` nowadays say that it's equivalent to `run(...).returncode`. If that's the case, then I would expect the warning in `call()` to apply also to `run()`. Or is the warning nowadays outdated altogether?
msg315526 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2018-04-20 17:26
If the goal is just to suppress stdout, that's what passing subprocess.DEVNULL is for (doesn't exist in Py2, but opening os.devnull and passing that is a slightly higher overhead equivalent).

subprocess.run includes a call to communicate as part of its default behavior, and stores its results, so call() isn't quite equivalent to run().returncode when PIPE was passed for standard handles, because call only includes an implicit call to wait, not communicate, and therefore pipes are not explicitly read and can block.

Basically, subprocess.run is deadlock-safe (because it uses communicate, not just wait), but if you don't care about the results, and the results might be huge, don't pass it PIPE for stdout/stderr (because it will store the complete outputs in memory, just like any use of communicate with PIPE).

The docs effectively tell you PIPE is safe; it returns a CompletedProcess object, and explicitly tells you that it has attributes that are (completely) populated based on whether capture was requested. If it had such attributes and still allowed deadlocks, it would definitely merit a warning.
msg315651 - (view) Author: Pekka Klärck (pekka.klarck) Date: 2018-04-23 08:23
My goal is to read stdout. It's good to hear `subprocess.run()` is deadlock-safe and I can use it safely. Making the docs explicit about it so that others know it's safe would in my opinion be a good idea as well.

Casual users don't know `run()` it uses `communicate()`, not `wait()`, internally, or even that this would mean it cannot deadlock. The current situation when the docs say that `call()` shouldn't be used with `stdout=PIPE` and that `call(...)` is equivalent to `run(...).returncode` indicates `stdout=PIPE` is unsafe with `run()` as well.

A separate questions is that if `call(...)` is equivalent to `run(...).returncode`, should it also be implemented that way. Based on this discussion it would avoid the problem with `stdout=PIPE` also in that case.
msg338636 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2019-03-23 00:53
This is a regression in the 3.7+ documentation. It previously said “To [capture output], pass PIPE for the ‘stdout’ and/or ‘stderr’ arguments”. This was removed by Bo Bayles in Issue 32102.
msg338652 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2019-03-23 07:40
New changeset 7a2e84c3488cfd6c108c6b41ff040825f1757566 by Gregory P. Smith in branch 'master':
bpo-33319: Clarify subprocess call docs. (GH-12508)
https://github.com/python/cpython/commit/7a2e84c3488cfd6c108c6b41ff040825f1757566
msg338654 - (view) Author: miss-islington (miss-islington) Date: 2019-03-23 07:46
New changeset 9cdac5ced68f1d6ef5e1eee7552bb200b46adc23 by Miss Islington (bot) in branch '3.7':
bpo-33319: Clarify subprocess call docs. (GH-12508)
https://github.com/python/cpython/commit/9cdac5ced68f1d6ef5e1eee7552bb200b46adc23
History
Date User Action Args
2019-03-23 07:51:13gregory.p.smithsetstatus: open -> closed
nosy: - miss-islington

resolution: fixed
stage: patch review -> commit review
2019-03-23 07:46:17miss-islingtonsetnosy: + miss-islington
messages: + msg338654
2019-03-23 07:40:47miss-islingtonsetpull_requests: + pull_request12460
2019-03-23 07:40:33gregory.p.smithsetmessages: + msg338652
2019-03-23 07:25:47gregory.p.smithsetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request12459
2019-03-23 00:53:20martin.pantersetversions: + Python 3.7, Python 3.8, Python 3.9
nosy: + martin.panter, bbayles, gregory.p.smith

messages: + msg338636

keywords: + 3.7regression
stage: needs patch
2018-04-23 08:23:44pekka.klarcksetmessages: + msg315651
2018-04-20 17:26:03josh.rsetnosy: + josh.r
messages: + msg315526
2018-04-20 13:05:37pekka.klarcksetassignee: docs@python

components: + Documentation
nosy: + docs@python
2018-04-20 13:05:14pekka.klarckcreate