GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — GH-88050: fix race in closing subprocess pipe in asyncio by kumaraditya303 · Pull Request #97951 · python/cpython

Conversation

kumaraditya303 commented Oct 6, 2022 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Issue: Cannot cleanly kill a subprocess using high-level asyncio APIs #88050

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          fix another race

kumaraditya303 requested review from 1st1, asvetlov and gvanrossum as code owners

October 6, 2022 05:01

bedevere-bot added the awaiting review label

Oct 6, 2022

kumaraditya303 added topic-asyncio skip news needs backport to 3.11 only security fixes labels

Oct 6, 2022

kumaraditya303 commented Oct 6, 2022

Copy link

Copy Markdown

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()

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gvanrossum approved these changes

Oct 6, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels

Oct 6, 2022

kumaraditya303 assigned gvanrossum

Oct 6, 2022

blockchainGuru1018 approved these changes

Oct 6, 2022

View reviewed changes

gvanrossum approved these changes

Oct 6, 2022

View reviewed changes

gvanrossum left a comment

Copy link

Copy Markdown

Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gvanrossum merged commit e2e6b95 into python:main

Oct 6, 2022

miss-islington commented Oct 6, 2022

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the awaiting merge label

Oct 6, 2022

miss-islington commented Oct 6, 2022

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kumaraditya303 deleted the race branch

October 6, 2022 17:30

kumaraditya303 commented Oct 6, 2022

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 commented Oct 6, 2022

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot commented Oct 6, 2022

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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


          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 <[email protected]>

carljm added a commit to carljm/cpython that referenced this pull request

Oct 6, 2022


          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


          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


          [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 <[email protected]>

carljm added a commit to carljm/cpython that referenced this pull request

Oct 8, 2022


          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


          pythonGH-88050: fix race in closing subprocess pipe in asyncio (pytho…

bf234f5

…n#97951)

Check for None when iterating over `self._pipes.values()`.

Reviewers

gvanrossum

gvanrossum approved these changes

1st1

Awaiting requested review from 1st1

1st1 is a code owner

asvetlov

Awaiting requested review from asvetlov

asvetlov is a code owner

+1 more reviewer

blockchainGuru1018

blockchainGuru1018 approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

gvanrossum

Labels

skip news topic-asyncio

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

kumaraditya303 commented Oct 6, 2022 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kumaraditya303 commented Oct 6, 2022

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Oct 6, 2022

Uh oh!

miss-islington commented Oct 6, 2022

Uh oh!

kumaraditya303 commented Oct 6, 2022

Uh oh!

miss-islington commented Oct 6, 2022

Uh oh!

bedevere-bot commented Oct 6, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kumaraditya303 commented Oct 6, 2022 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.