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.

Sign up for GitHub

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-88050: fix race in closing subprocess pipe in asyncio #97951

Merged
merged 1 commit into from
Oct 6, 2022
Merged

GH-88050: fix race in closing subprocess pipe in asyncio #97951

merged 1 commit into from
Oct 6, 2022

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Oct 6, 2022 •
edited by bedevere-bot
Loading

@kumaraditya303
Copy link
Contributor Author

@gvanrossum: In rare cases it is possible that pipe was closed before process exited, so this skips closing if is it is already closed. It also aligns it with the close method which skips it too.

cpython/Lib/asyncio/base_subprocess.py

Lines 96 to 104 in 0d68879

def close(self):
if self._closed:
return
self._closed = True
for proto in self._pipes.values():
if proto is None:
continue
proto.pipe.close()

gvanrossum
gvanrossum approved these changes Oct 6, 2022
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I reviewed this part in the original PR I did think "how do we know these pipe values aren't None" but didn't express it. I now checked, and the other two places that iterate over self._pipes.values() do check for None so this fix seems correct.

@gvanrossum gvanrossum merged commit e2e6b95 into python:main Oct 6, 2022
@miss-islington
Copy link
Contributor

Thanks @kumaraditya303 for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry @kumaraditya303 and @gvanrossum, I had trouble checking out the 3.11 backport branch.
Please backport using cherry_picker on command line.
cherry_picker e2e6b95c0342247ed1a761b6e149ac579a8722dd 3.11

@kumaraditya303 kumaraditya303 deleted the race branch October 6, 2022 17:30
@kumaraditya303
Copy link
Contributor Author

When I reviewed this part in the original PR I did think "how do we know these pipe values aren't None" but didn't express it.

Yeah, it shouldn't happen in the usual case and confirmed as the CI is green without this but still may happen so better be on the safe side.

@kumaraditya303 kumaraditya303 added needs backport to 3.11 only security fixes and removed needs backport to 3.11 only security fixes labels Oct 6, 2022
@miss-islington
Copy link
Contributor

Thanks @kumaraditya303 for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

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

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Oct 6, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 6, 2022
@kumaraditya303 @miss-islington
pythonGH-88050: fix race in closing subprocess pipe in asyncio (pytho…
2e5886a 
…nGH-97951)

Check for None when iterating over `self._pipes.values()`.
(cherry picked from commit e2e6b95)

Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
carljm added a commit to carljm/cpython that referenced this pull request Oct 6, 2022
@carljm
Merge branch 'main' into dictwatch
7938d6f 
* main:
  pythonGH-88050: fix race in closing subprocess pipe in asyncio  (python#97951)
  pythongh-93738: Disallow pre-v3 syntax in the C domain (python#97962)
  pythongh-95986: Fix the example using match keyword (python#95989)
  pythongh-97897: Prevent os.mkfifo and os.mknod segfaults with macOS 13 SDK (pythonGH-97944)
  pythongh-94808: Cover `PyUnicode_Count` in CAPI (python#96929)
  pythongh-94808: Cover `PyObject_PyBytes` case with custom `__bytes__` method (python#96610)
  pythongh-95691: Doc BufferedWriter and BufferedReader (python#95703)
  pythonGH-88968: Add notes about socket ownership transfers (python#97936)
  pythongh-96865: [Enum] fix Flag to use CONFORM boundary (pythonGH-97528)
lysnikolaou pushed a commit to lysnikolaou/cpython that referenced this pull request Oct 6, 2022
@kumaraditya303 @lysnikolaou
pythonGH-88050: fix race in closing subprocess pipe in asyncio (pytho…
25c1004 
…n#97951)

Check for None when iterating over `self._pipes.values()`.
ambv pushed a commit that referenced this pull request Oct 6, 2022
@miss-islington @kumaraditya303
[3.11] GH-88050: fix race in closing subprocess pipe in asyncio (GH-9…
bd3dcb3 
…7951) (#97978)

Check for None when iterating over `self._pipes.values()`.
(cherry picked from commit e2e6b95)

Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
carljm added a commit to carljm/cpython that referenced this pull request Oct 8, 2022
@carljm
Merge branch 'main' into typewatch
1af2a98 
* main: (53 commits)
  pythongh-94808: Coverage: Test that maximum indentation level is handled (python#95926)
  pythonGH-88050: fix race in closing subprocess pipe in asyncio  (python#97951)
  pythongh-93738: Disallow pre-v3 syntax in the C domain (python#97962)
  pythongh-95986: Fix the example using match keyword (python#95989)
  pythongh-97897: Prevent os.mkfifo and os.mknod segfaults with macOS 13 SDK (pythonGH-97944)
  pythongh-94808: Cover `PyUnicode_Count` in CAPI (python#96929)
  pythongh-94808: Cover `PyObject_PyBytes` case with custom `__bytes__` method (python#96610)
  pythongh-95691: Doc BufferedWriter and BufferedReader (python#95703)
  pythonGH-88968: Add notes about socket ownership transfers (python#97936)
  pythongh-96865: [Enum] fix Flag to use CONFORM boundary (pythonGH-97528)
  pythongh-65961: Raise `DeprecationWarning` when `__package__` differs from `__spec__.parent` (python#97879)
  docs(typing): add "see PEP 675" to LiteralString (python#97926)
  pythongh-97850: Remove all known instances of module_repr() (python#97876)
  I changed my surname early this year (python#96671)
  pythongh-93738: Documentation C syntax (:c:type:<C type> -> :c:expr:<C type>) (python#97768)
  pythongh-91539: improve performance of get_proxies_environment  (python#91566)
  build(deps): bump actions/stale from 5 to 6 (python#97701)
  pythonGH-95172 Make the same version `versionadded` oneline (python#95172)
  pythongh-88050: Fix asyncio subprocess to kill process cleanly when process is blocked (python#32073)
  pythongh-93738: Documentation C syntax (Function glob patterns -> literal markup) (python#97774)
  ...
mpage pushed a commit to mpage/cpython that referenced this pull request Oct 11, 2022
@kumaraditya303 @mpage
pythonGH-88050: fix race in closing subprocess pipe in asyncio (pytho…
bf234f5 
…n#97951)

Check for None when iterating over `self._pipes.values()`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blockchainGuru1018 blockchainGuru1018 blockchainGuru1018 approved these changes

@gvanrossum gvanrossum gvanrossum approved these changes

@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

Assignees

@gvanrossum gvanrossum

Labels
skip news topic-asyncio
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kumaraditya303 @miss-islington @bedevere-bot @gvanrossum @blockchainGuru1018