| msg227095 - (view) |
Author: juj (juj) |
Date: 2014-09-19 13:56 |
| On Windows, write a.py: import subprocess def ccall(cmdline, stdout, stderr): proc = subprocess.Popen(['python', 'b.py'], stdout=subprocess.PIPE, stderr=subprocess.PIPE) proc.communicate() if proc.returncode != 0: raise subprocess.CalledProcessError(proc.returncode, cmdline) return 0 # To fix subprocess.check_call, uncomment the following, which is functionally equivalent: # subprocess.check_call = ccall subprocess.check_call(['python', 'b.py'], stdout=subprocess.PIPE, stderr=subprocess.PIPE) print 'Finished!' Then write b.py: import sys str = 'aaa' for i in range(0,16): str = str + str for i in range(0,2): print >> sys.stderr, str for i in range(0,2): print str Finally, run 'python a.py'. The application will hang. Uncomment the specicied line to fix the execution. This is a documented failure on the python subprocess page, but why not just fix it up directly in python itself? One can think that modifying stdout or stderr is not the intent for subprocess.check_call, but python certainly should not hang because of that. |
|
|
| msg227096 - (view) |
Author: juj (juj) |
Date: 2014-09-19 14:05 |
| The same observation applies to subprocess.call() as well. |
|
|
| msg227138 - (view) |
Author: Akira Li (akira) * |
Date: 2014-09-20 00:35 |
| > This is a documented failure on the python subprocess page, > but why not just fix it up directly in python itself? If you want to discard the output; you could use: check_call(args, stdin=DEVNULL, stdout=DEVNULL, stderr=STDOUT) check_call() passes its parameters to Popen() as is. The only parameter it knows about is args that is used to raise an exception. Do you want check_call() to inspect the parameters and to do something about stdout=PIPE, stderr=PIPE? Where "something" could be: - nothing -- the current behavior: everything works until the child process produces enough output to fill any of OS pipe buffers as documented - call proc.communicate() -- store (unlimited) output in memory instead of just hanging: everything works slowly until the system runs out of memory - replace with DEVNULL -- "do what I mean" behavior: inconsistent with the direct Popen() call - raise ValueError with informative error message (about DEVNULL option) after issueing a DeprecationWarning for a release: it fixes this particular misuse of check_call(). Are there other common "wrong in every case" check_call() parameters? |
|
|
| msg227148 - (view) |
Author: juj (juj) |
Date: 2014-09-20 08:51 |
| Very good question akira. In one codebase where I have fixed this kind of bug, see https://github.com/kripken/emscripten/commit/1b2badd84bc6f54a3125a494fa38a51f9dbb5877 https://github.com/kripken/emscripten/commit/2f048a4e452f5bacdb8fa31481c55487fd64d92a the intended usage by the original author had certainly been to throw in a PIPE just to mute both stdout and stderr output, and there was no intent to capture the results or anything. I think passing PIPE to those is meaningless, since they effectively behave as "throw the results away", since they are not returned. Throwing an exception might be nice, but perhaps that would break existing codebases and therefore is not good to add(?). Therefore I think the best course of action would be to do what is behaviorally as developer intends: "please treat as if stdout and stderr had been captured to a pipe, and throw those pipes away, since they aren't returned.", so your third option, while inconsistent with direct Popen(), sounds most correct in practice. What do you think? I am not currently aware of other such cases, although it would be useful to go through the docs and recheck the commit history of when that documentation note was added in to see if there was more related discussions that occurred. |
|
|
| msg227206 - (view) |
Author: Akira Li (akira) * |
Date: 2014-09-21 08:10 |
| > What do you think? I would prefer to deprecate PIPE argument for subprocess.call(): issue DeprecationWarning in 3.5 and raise ValueError in 3.6+ I've uploaded a patch that issues the warning. |
|
|
| msg227208 - (view) |
Author: juj (juj) |
Date: 2014-09-21 08:32 |
| Hmm, that path does it for stdout=PIPE in subprocess.call only? It could equally apply to stderr=PIPE in subprocess.call as well, and also to both stdout=PIPE and stderr=PIPE in subprocess.check_call? |
|
|
| msg227217 - (view) |
Author: Akira Li (akira) * |
Date: 2014-09-21 14:03 |
| @juj: DeprecationWarning is generated if PIPE is passed to call() as any positional or keyword argument in particular stdin, stdout, stderr. It also applies to check_call() that uses call() internally. |
|
|
| msg227224 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2014-09-21 19:16 |
| The first place to warn uses about dangerous function calls is the documentation, and your patch doesn't touch the documentation. You can for example suggest to use check_output(), getstatusouptut() or getoutput(). |
|
|
| msg227251 - (view) |
Author: Akira Li (akira) * |
Date: 2014-09-22 04:02 |
| Victor, the message in my patch is copied almost verbatim from the current subprocess' documentation [1] [1] https://hg.python.org/cpython/file/850a62354402/Doc/library/subprocess.rst#l57 People use `call(cmd, stdout=PIPE)` as a *broken* way to suppress output i.e., when they actually want `call(cmd, stdout=DEVNULL)` The issue with `call(cmd, stdout=PIPE)` that it *appears* to work if cmd doesn't produce much output i.e., it might work in tests but may hang in production. It is unrelated to check_output(), getstatusouptut() or getoutput(). |
|
|
| msg243615 - (view) |
Author: juj (juj) |
Date: 2015-05-19 18:11 |
| This issue still reads open, but there has not been activity in a long time. May I ask what is the latest status on this? Also, any chance whether this will be part of Python 2.x? |
|
|
| msg244644 - (view) |
Author: Martin Panter (martin.panter) * |
Date: 2015-06-02 02:48 |
| I agree with the deprecation idea. The parameter checking logic doesn’t seem right though; see Reitveld. Also, I would have made the warning specify exactly what is deprecated, in case the stack trace doesn’t identify the function, which I think would always happen with check_call(). Also be less specific about future changes, unless there is clear consensus to make this change in 3.6. Maybe something like: "Passing PIPE to call() and check_call() is deprecated; use DEVNULL instead to discard output or provide empty input" Since 3.5 is now in the beta phase, would adding this deprecation be allowed, or should it be deferred to the 3.6 branch? Also, I’m not sure what the policy is for Python 2. Maybe it would be acceptable as a Python 3 compatibility warning, triggered by the “python2 -3” option; I dunno. |
|
|
| msg244645 - (view) |
Author: Matthias Bussonnier (mbussonn) * |
Date: 2015-06-02 04:16 |
| 3.5 have `subprocess.run`[1] that is much saner to use, and what you want to use in most cases. `call` and `check_call` docs even mention run. [1]: https://docs.python.org/3.5/library/subprocess.html#subprocess.run |
|
|
| msg245586 - (view) |
Author: Akira Li (akira) * |
Date: 2015-06-21 09:44 |
| Martin, thank you for the review. As Matthias mentioned, the introduction of subprocess.run() perhaps deprecates this issue: old api should be left alone to avoid breaking old code, new code should use new api, those who need old api (e.g., to write 2/3 compatible code) are expected to read the docs that already contain necessary warnings). |
|
|
| msg380796 - (view) |
Author: Irit Katriel (iritkatriel) * |
Date: 2020-11-11 23:21 |
| Any objections to closing this? If the old API is going to be deprecated I think that's a topic for another issue. |
|
|