Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-130925: Addclose()
method toasyncio.StreamReader
#130929
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
When creating a sub-process using `asyncio.create_subprocess_exec()`,it returns a `Process` instance that has a `stdout` property. Thisproperty is intended to be an asyncio version of the `stdout` propertyof the `Popen` instance from the `subprocess` module.An important aspect of `Popen.stdout` property is that you can closeit. This is a signal to the sub-process that is generating output thatit should cleanly terminate. This is a common pattern in processesused in shell pipelines. Indeed, the object located at `Popen.stdout`has a `close()` method. This pattern is demonstrated below:```pythonimport subprocessproc = subprocess.Popen(["yes"], stdout=subprocess.PIPE) # start subprocessdata = proc.stdout.read(4096) # get dataproc.stdout.close() # signal to process to cleanly shutdownproc.wait() # wait for shutdown```Unfortunately this pattern cannot be reproduced easily with the`stdout` property of the `Process` instance returned from`asyncio.create_subprocess_exec()` because `stdout` is an instance of`StreamReader` which does not have the `close()` method.This change adds a `close()` method to the `StreamReader` class sothat asyncio version of the `subprocess` module may support thispattern of managing sub-processes. This change is consistent with theasyncio ecosystem as the companion `StreamWriter` class already has a`close()` method, along with other methods that expose its inner"transport" object. It's also trivial to implement, since it'sessentially a wrapper method around the inner transport object's`close()` method.
Doc/library/asyncio-stream.rst Outdated
@@ -290,6 +290,10 @@ StreamReader | |||
Return ``True`` if the buffer is empty and :meth:`feed_eof` | |||
was called. | |||
.. method:: close() | |||
Invoke ``close()`` on the underlying transport (if one exists). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it is not clear what "what" means here: "close()
method" or "underlying transport"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A transport is a pervasive concept throughout the asyncio package.StreamReader
objects usually have an internal transport member. I can add a link to the transport documentation page here to make that more clear. Or what do you suggest?
@@ -35,6 +35,17 @@ | |||
'data = sys.stdin.buffer.read()', | |||
'sys.stdout.buffer.write(data)'))] | |||
# Program generating infinite data | |||
PROGRAM_YES = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is there a reason to make this a global var? Maybe we can just define it in test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No reason other than consistency withPROGRAM_CAT
andPROGRAM_BLOCKED
. I can make it a local variable.
proc = await asyncio.create_subprocess_exec(*PROGRAM_YES, | ||
stdout=asyncio.subprocess.PIPE) | ||
try: | ||
# just make sure the program has executed correctly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
# just make sure the program has executed correctly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I added that comment so it's clear why those otherwise unnecessary lines of code are there. They aren't conceptually part of the unit test though. I can be more clear.
# we are testing that the following method exists and | ||
# has the intended effect of signaling the sub-process to terminate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
# we are testing that the following method exists and | |
# has the intended effect of signaling the sub-process to terminate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same as before, the comment is there to make it explicit what the intent of the test is and the code included in the test.
:func:`asyncio.StreamReader.close` now exists so that it's possible to | ||
signal to sub-processes executed via :func:`asyncio.create_subprocess_exec` | ||
that they may cease generating output and exit cleanly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
:func:`asyncio.StreamReader.close` now exists so that it's possible to | |
signal to sub-processes executed via:func:`asyncio.create_subprocess_exec` | |
that they may cease generating output and exit cleanly. | |
Add:func:`asyncio.StreamReader.close` method. It can signal to sub-processes executed via:func:`asyncio.create_subprocess_exec` | |
that they may cease generating output and exit cleanly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think the "so" was a typo? Otherwise I can change this
Uh oh!
There was an error while loading.Please reload this page.
When creating a sub-process using
asyncio.create_subprocess_exec()
, it returns aProcess
instance that has astdout
property. This property is intended to be an asyncio version of thestdout
property of thePopen
instance from thesubprocess
module.An important aspect of
Popen.stdout
property is that you can close it. This is a signal to the sub-process that is generating output that it should cleanly terminate. This is a common pattern in processes used in shell pipelines. Indeed, the object located atPopen.stdout
has aclose()
method. This pattern is demonstrated below:Unfortunately this pattern cannot be reproduced easily with the
stdout
property of theProcess
instance returned fromasyncio.create_subprocess_exec()
becausestdout
is an instance ofStreamReader
which does not have theclose()
method.This change adds a
close()
method to theStreamReader
class so that asyncio version of thesubprocess
module may support this pattern of managing sub-processes. This change is consistent with the asyncio ecosystem as the companionStreamWriter
class already has aclose()
method, along with other methods that expose its inner "transport" object. It's also trivial to implement, since it's essentially a wrapper method around the inner transport object'sclose()
method.close()
method toasyncio.StreamReader
#130925📚 Documentation preview 📚:https://cpython-previews--130929.org.readthedocs.build/