gh-83925: make asyncio.subprocess communicate similar to non-asyncio #18650

marmarek · 2020-02-24T22:35:31Z

subprocess's communicate(None) closes stdin of the child process, after
sending no (extra) data. Make asyncio variant do the same.
This fixes issues with processes that waits for EOF on stdin before
continuing.

https://bugs.python.org/issue39744

Issue: asyncio.subprocess's communicate(None) does not close stdin #83925

the-knights-who-say-ni · 2020-02-24T22:35:34Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@marmarek

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

marmarek · 2020-02-24T22:38:57Z

(CLA just signed, probably state not refreshed yet - will recheck tomorrow)

codecov · 2020-02-25T01:31:43Z

Codecov Report

Merging #18650 into master will decrease coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #18650      +/-   ##
==========================================
- Coverage   82.12%   82.07%   -0.06%     
==========================================
  Files        1956     1955       -1     
  Lines      589451   584109    -5342     
  Branches    44457    44458       +1     
==========================================
- Hits       484065   479380    -4685     
+ Misses      95737    95102     -635     
+ Partials     9649     9627      -22

Impacted Files	Coverage Δ
Lib/distutils/tests/test_bdist_rpm.py	`30.00% <0.00%> (-65.00%)`	⬇️
Lib/distutils/command/bdist_rpm.py	`7.63% <0.00%> (-56.88%)`	⬇️
Modules/_decimal/libmpdec/umodarith.h	`80.76% <0.00%> (-19.24%)`	⬇️
Lib/test/test_urllib2net.py	`76.92% <0.00%> (-13.85%)`	⬇️
Lib/test/test_smtpnet.py	`78.57% <0.00%> (-7.15%)`	⬇️
Lib/ftplib.py	`63.85% <0.00%> (-6.06%)`	⬇️
Lib/test/test_ftplib.py	`87.11% <0.00%> (-4.72%)`	⬇️
Tools/scripts/db2pickle.py	`17.82% <0.00%> (-3.97%)`	⬇️
Tools/scripts/pickle2db.py	`16.98% <0.00%> (-3.78%)`	⬇️
Lib/test/test_socket.py	`71.94% <0.00%> (-3.77%)`	⬇️
... and 327 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8af4712...fbb5546. Read the comment docs.

willingc

Hi @marmarek, Thanks for the PR. If you wish to move this PR forward, please fix the CI errors. Thanks!

marmarek · 2023-04-27T00:31:27Z

Hi @marmarek, Thanks for the PR. If you wish to move this PR forward, please fix the CI errors. Thanks!

Done

gvanrossum

I think the logic is correct. This requires a test and a small doc update though. Can you take care of those?

gvanrossum · 2023-04-27T00:57:15Z

Misc/NEWS.d/next/Library/2020-02-25-00-43-22.bpo-39744.hgK689.rst

@@ -0,0 +1 @@
+Make func:``asyncio.subprocess.Process.communicate`` close subprocess's stdin even when called with input=None


Suggested change

Make func:``asyncio.subprocess.Process.communicate`` close subprocess's stdin even when called with input=None

Make :func:`asyncio.subprocess.Process.communicate` close the subprocess's stdin even when called with `input=None`.

Oh, there was also a missing : before func. Look at other uses of this syntax in the Doc tree for comparison.

I just changed single quotes to double ones, because the build failed and said I need to use double ones...

gvanrossum · 2023-04-27T02:09:21Z

(And by "small doc changes" I didn't mean the NEWS file edit; I would like the docs for .communicate() to clarify this.)

willingc · 2023-04-27T02:14:36Z

@marmarek The docs page is at: https://github.com/python/cpython/blob/main/Doc/library/asyncio-subprocess.rst

marmarek · 2023-04-27T02:31:33Z

It seems the doc already describes the behavior after this patch:

   Interact with process:

   1. send data to *stdin* (if *input* is not ``None``);

And also the list of differences vs subprocess.Popen.communicate() do not mention different handling of input parameter. Should I add a note that wasn't the case in older versions? If so, I guess that should be rather in branch for such older versions, no?

… one subprocess's communicate(None) closes stdin of the child process, after sending no (extra) data. Make asyncio variant do the same. This fixes issues with processes that waits for EOF on stdin before continuing.

gvanrossum · 2023-04-27T04:19:08Z

It seems the doc already describes the behavior after this patch:
   Interact with process:

   1. send data to *stdin* (if *input* is not ``None``);
And also the list of differences vs subprocess.Popen.communicate() do not mention different handling of input parameter. Should I add a note that wasn't the case in older versions? If so, I guess that should be rather in branch for such older versions, no?

For now, let's just add a versionchanged note to the description of communicate(). And we won't backport the fix. We can update the 3.11 (and before) docs in a separate PR.

gvanrossum · 2023-04-27T04:20:09Z

Also, please don't force-push, it is confusing for reviewers. When we merge a PR we squash commits anyways.

gvanrossum

Still to do:

Fix the NEWS file
Add versionchanged note

gvanrossum

Awesome!

gvanrossum · 2023-04-28T00:31:23Z

I'm not sure this should be backported to 3.11; people might have existing workarounds for the issue that could be broken. (Can you close stdin twice?)

marmarek requested review from 1st1 and asvetlov as code owners February 24, 2020 22:35

the-knights-who-say-ni added the CLA not signed label Feb 24, 2020

bedevere-bot added the awaiting review label Feb 24, 2020

marmarek force-pushed the asyncio-communicate-close branch 2 times, most recently from d23078a to fbb5546 Compare February 25, 2020 00:45

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Feb 26, 2020

marmarek mannequin mentioned this pull request Apr 10, 2022

asyncio.subprocess's communicate(None) does not close stdin #83925

Open

ezio-melotti removed the CLA signed label Jul 13, 2022

kumaraditya303 closed this Sep 29, 2022

kumaraditya303 reopened this Sep 29, 2022

kumaraditya303 added the topic-asyncio label Sep 29, 2022

willingc reviewed Apr 26, 2023

View reviewed changes

willingc added awaiting changes and removed awaiting review labels Apr 26, 2023

marmarek force-pushed the asyncio-communicate-close branch from c2d9dc3 to 4bf5647 Compare April 26, 2023 23:56

marmarek requested review from gvanrossum and kumaraditya303 as code owners April 26, 2023 23:56

gvanrossum changed the title ~~bpo-39744: make asyncio.subprocess communicate similar to non-asyncio~~ gh-83925: make asyncio.subprocess communicate similar to non-asyncio Apr 27, 2023

gvanrossum reviewed Apr 27, 2023

View reviewed changes

marmarek force-pushed the asyncio-communicate-close branch from d863a4b to 0272411 Compare April 27, 2023 02:34

marmarek force-pushed the asyncio-communicate-close branch from 0272411 to 9e9e37b Compare April 27, 2023 04:17

gvanrossum reviewed Apr 27, 2023

View reviewed changes

marmarek added 2 commits April 27, 2023 12:54

bpo-39744: update docs

0ff1c2e

bpo-39744: news file update

4efe16b

marmarek force-pushed the asyncio-communicate-close branch from f0b8dfc to 4efe16b Compare April 27, 2023 10:54

bpo-39744: news file update

d3223a3

gvanrossum approved these changes Apr 28, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Apr 28, 2023

gvanrossum merged commit 67d140d into python:main Apr 28, 2023
19 of 20 checks passed

bedevere-bot removed the awaiting merge label Apr 28, 2023

gh-83925: make asyncio.subprocess communicate similar to non-asyncio #18650

gh-83925: make asyncio.subprocess communicate similar to non-asyncio #18650

marmarek commented Feb 24, 2020 •

edited by bedevere-bot

the-knights-who-say-ni commented Feb 24, 2020

marmarek commented Feb 24, 2020

codecov bot commented Feb 25, 2020

willingc left a comment

marmarek commented Apr 27, 2023

gvanrossum left a comment

gvanrossum Apr 27, 2023 •

edited

marmarek Apr 27, 2023

gvanrossum commented Apr 27, 2023

willingc commented Apr 27, 2023

marmarek commented Apr 27, 2023

gvanrossum commented Apr 27, 2023

gvanrossum commented Apr 27, 2023

gvanrossum left a comment

gvanrossum left a comment

gvanrossum commented Apr 28, 2023

		@@ -0,0 +1 @@
		Make func:``asyncio.subprocess.Process.communicate`` close subprocess's stdin even when called with input=None

	Make func:``asyncio.subprocess.Process.communicate`` close subprocess's stdin even when called with input=None
	Make :func:`asyncio.subprocess.Process.communicate` close the subprocess's stdin even when called with `input=None`.

gh-83925: make asyncio.subprocess communicate similar to non-asyncio #18650

gh-83925: make asyncio.subprocess communicate similar to non-asyncio #18650

Conversation

marmarek commented Feb 24, 2020 • edited by bedevere-bot

the-knights-who-say-ni commented Feb 24, 2020

CLA Missing

marmarek commented Feb 24, 2020

codecov bot commented Feb 25, 2020

Codecov Report

willingc left a comment

Choose a reason for hiding this comment

marmarek commented Apr 27, 2023

gvanrossum left a comment

Choose a reason for hiding this comment

gvanrossum Apr 27, 2023 • edited

Choose a reason for hiding this comment

marmarek Apr 27, 2023

Choose a reason for hiding this comment

gvanrossum commented Apr 27, 2023

willingc commented Apr 27, 2023

marmarek commented Apr 27, 2023

gvanrossum commented Apr 27, 2023

gvanrossum commented Apr 27, 2023

gvanrossum left a comment

Choose a reason for hiding this comment

gvanrossum left a comment

Choose a reason for hiding this comment

gvanrossum commented Apr 28, 2023

marmarek commented Feb 24, 2020 •

edited by bedevere-bot

gvanrossum Apr 27, 2023 •

edited