Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

asyncio subprocess stdin/out/err can be filehandles, but this is undocumented #105857

Closed
bewinsnw opened this issue Jun 16, 2023 · 4 comments
Closed
Labels
docs Documentation in the Doc dir topic-asyncio

Comments

@bewinsnw
Copy link

bewinsnw commented Jun 16, 2023

Documentation

I was trying to write some asyncio subprocess code to pipe from one process to another, struggled until I found examples with os.pipe(). But going back to the docs I was confused because passing a filehandle isn't mentioned as an option. Looking at the underlying code, I see it definitely is supported, there's a check for the parameter being an integer and the handle is used directly.

The incorrect documentation is here

* *stdin* can be any of these:
* a file-like object
* the :const:`subprocess.PIPE` constant (default) which will create a new
pipe and connect it,
* the value ``None`` which will make the subprocess inherit the file
descriptor from this process
* the :const:`subprocess.DEVNULL` constant which indicates that the
special :data:`os.devnull` file will be used
* *stdout* can be any of these:
* a file-like object
* the :const:`subprocess.PIPE` constant (default) which will create a new
pipe and connect it,
* the value ``None`` which will make the subprocess inherit the file
descriptor from this process
* the :const:`subprocess.DEVNULL` constant which indicates that the
special :data:`os.devnull` file will be used
* *stderr* can be any of these:
* a file-like object
* the :const:`subprocess.PIPE` constant (default) which will create a new
pipe and connect it,
* the value ``None`` which will make the subprocess inherit the file
descriptor from this process
* the :const:`subprocess.DEVNULL` constant which indicates that the
special :data:`os.devnull` file will be used
* the :const:`subprocess.STDOUT` constant which will connect the standard
error stream to the process' standard output stream

So instead of

   * *stdin* can be any of these:

     * a file-like object
     * the :const:`subprocess.PIPE` constant (default) which will create a new
       pipe and connect it,...

I'd suggest:

   * *stdin* can be any of these:
  
     * a file-like object
     * an existing file descriptor (a positive integer), for example those created with :meth:`os.pipe()`,
     * the :const:`subprocess.PIPE` constant (default) which will create a new
       pipe and connect it,...

(and similarly for stdout and stderr)

Linked PRs

@bewinsnw bewinsnw added the docs Documentation in the Doc dir label Jun 16, 2023
@github-project-automation github-project-automation bot moved this to Todo in asyncio Jun 16, 2023
@gvanrossum
Copy link
Member

I'm not sure if this is just a lack of documentation or whether it is intentional, to discourage using raw file descriptors? There might be some problems in edge cases (e.g. sockets on Windows). If you have a simple example of how you used this successfully that would also help.

@bewinsnw
Copy link
Author

example:

import asyncio
import os

async def test():
    r,w = os.pipe()
    p1 = asyncio.create_subprocess_shell("seq 1 100000", stdout=w)
    p2 = asyncio.create_subprocess_shell("wc", stdin=r)
    await asyncio.gather(p1, p2)

asyncio.run(test())
$ python3 spike.py
  100000  100000  588895                                                                                                                                                                     

(the actual code I'm using is a fair bit more complex and takes care of closing handles etc).

I think it's just an omission, not intentional. The asyncio subprocess functions are mostly just wrappers for Popen, which is the thing handling the file handles; and it's handled in both unix and windows variants.

The non-asyncio subprocess documents that file descriptors are ok: https://docs.python.org/3/library/subprocess.html#frequently-used-arguments "stdin, stdout and stderr specify the executed program’s standard input, standard output and standard error file handles, respectively. Valid values are PIPE, DEVNULL, an existing file descriptor (a positive integer), an existing file object with a valid file descriptor, and None."

In the asyncio subprocess implementation, _UnixSubprocessTransport passes everything but subprocess.PIPE to Popen:

self._proc = subprocess.Popen(
args, shell=shell, stdin=stdin, stdout=stdout, stderr=stderr,
universal_newlines=False, bufsize=bufsize, **kwargs)

Popen calls _get_handles:
errread, errwrite) = self._get_handles(stdin, stdout, stderr)

which handles the int filehandles case: https://github.com/python/cpython/blob/main/Lib/subprocess.py#L1704-L1705

_WindowsSubprocessTransport is even simpler and passes all of the stream arguments to Popen: https://github.com/python/cpython/blob/1858db7cbdbf41aa600c954c15224307bf81a258/Lib/asyncio/windows_events.py#L873C1-L875

@gvanrossum
Copy link
Member

Okay, makes sense. Would you like to submit a doc PR? (I wouldn't mind if the docs were refactored to avoid repeating everything three times, though maybe there are differences that make this tricky, so up to you.)

@Hels15
Copy link
Contributor

Hels15 commented Aug 16, 2023

I have added a pr here, extending the docs. #107986

ambv pushed a commit that referenced this issue Aug 22, 2023
…le handles (#107986)

stdin/out can be filehandles -> add to docs.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Aug 22, 2023
… be file handles (pythonGH-107986)

stdin/out can be filehandles -> add to docs.
(cherry picked from commit 13966da)

Co-authored-by: Hadházy Tamás <[email protected]>
ambv pushed a commit that referenced this issue Aug 22, 2023
…n be file handles (GH-107986) (#108332)

(cherry picked from commit 13966da)

Co-authored-by: Hadházy Tamás <[email protected]>
@github-project-automation github-project-automation bot moved this from Todo to Done in asyncio Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir topic-asyncio
Projects
Status: Done
Development

No branches or pull requests

4 participants