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

Jump to bottom

GH-100133: fix asyncio subprocess losing stderr and stdout output #100154

Merged
merged 9 commits into from Dec 21, 2022
Merged

GH-100133: fix asyncio subprocess losing stderr and stdout output #100154

merged 9 commits into from Dec 21, 2022
+18 −3

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Dec 10, 2022 •
edited by bedevere-bot

@kumaraditya303 kumaraditya303 added type-bug An unexpected behavior, bug, or error expert-asyncio needs backport to 3.11 labels Dec 10, 2022
@kumaraditya303 kumaraditya303 self-assigned this Dec 10, 2022
@kumaraditya303 kumaraditya303 requested a review from gvanrossum as a code owner Dec 10, 2022
@bedevere-bot bedevere-bot mentioned this pull request Dec 10, 2022
Closed
@kumaraditya303 kumaraditya303 changed the title GH-100133: fix asyncio subprocess losing stderr and stdout GH-100133: fix asyncio subprocess losing stderr and stdout output Dec 10, 2022
@kumaraditya303 kumaraditya303 linked an issue Dec 10, 2022 that may be closed by this pull request
asyncio subprocess stdout occasionally lost (3.11.0 → 3.11.1 regression) #100133
Closed
@python python deleted a comment from netlify bot Dec 10, 2022
@kumaraditya303 kumaraditya303 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 10, 2022
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 10, 2022
@python python deleted a comment from bedevere-bot Dec 10, 2022
@kumaraditya303 kumaraditya303 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 10, 2022
@bedevere-bot
Copy link

bedevere-bot commented Dec 10, 2022

🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit 3f55f14 🤖

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 10, 2022
@gvanrossum
Copy link
Member

gvanrossum commented Dec 10, 2022

Meta-comment: even though the commits will be squashed, as a reviewer I still appreciate more meaningful commit descriptions. (See also https://xkcd.com/1296/ :-)

I'll review later.

@kumaraditya303
Copy link
Contributor Author

kumaraditya303 commented Dec 11, 2022 •
edited

Ok will do next time, I was in hurry for this one and being not well doesn't help either :(

@gvanrossum
Copy link
Member

gvanrossum commented Dec 12, 2022

Hm, this should probably still be a draft. Also, I'd like to understand what change between 3.11.0 and 3.11.1 caused the regression, so I'm bisecting.

gvanrossum
gvanrossum reviewed Dec 12, 2022
View changes
Copy link
Member

@gvanrossum gvanrossum left a comment

There's some cleanup that needs to happen here.

I would also like to think more about whether the flow is now correct.

Lib/asyncio/base_subprocess.py Outdated Show resolved Hide resolved
Lib/test/test_asyncio/test_subprocess.py Show resolved Hide resolved
Misc/NEWS.d/next/Library/2022-12-10-08-36-07.gh-issue-100133.g-zQlp.rst Outdated Show resolved Hide resolved
@kumaraditya303 kumaraditya303 requested a review from gvanrossum Dec 15, 2022
@kumaraditya303
Copy link
Contributor Author

kumaraditya303 commented Dec 15, 2022

After this lands, I'll merge #100194.

Separately, I have created https://kumaraditya303.github.io/asyncio-coverage/ to track asyncio code coverage. It is updated on daily basis.

gvanrossum
gvanrossum approved these changes Dec 21, 2022
View changes
Copy link
Member

@gvanrossum gvanrossum left a comment

I've thought more about it and this does look like the right fix. Thanks for your patience! Go ahead and merge.

@gvanrossum
Copy link
Member

gvanrossum commented Dec 21, 2022

Looks like it should also be backported.

@kumaraditya303 kumaraditya303 merged commit a7715cc into python:main Dec 21, 2022
20 checks passed
@kumaraditya303 kumaraditya303 deleted the fix-subprocess branch Dec 21, 2022
@miss-islington
Copy link
Contributor

miss-islington commented Dec 21, 2022

Thanks @kumaraditya303 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@bedevere-bot
Copy link

bedevere-bot commented Dec 21, 2022

GH-100398 is a backport of this pull request to the 3.11 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 21, 2022
@kumaraditya303 @miss-islington
pythonGH-100133: fix asyncio subprocess losing stderr and `stdout…
cc25cd1 
…` output (pythonGH-100154)

(cherry picked from commit a7715cc)

Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
miss-islington added a commit that referenced this pull request Dec 21, 2022
@miss-islington @kumaraditya303
GH-100133: fix asyncio subprocess losing stderr and stdout outp…
ae8520c 
…ut (GH-100154)

(cherry picked from commit a7715cc)

Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gvanrossum gvanrossum

@1st1 1st1

@asvetlov asvetlov

Assignees

@kumaraditya303 kumaraditya303

Labels
expert-asyncio type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

asyncio subprocess stdout occasionally lost (3.11.0 → 3.11.1 regression)
4 participants
@kumaraditya303 @bedevere-bot @gvanrossum @miss-islington