Skip to content

gh-110205: Fix asyncio ThreadedChildWatcher._join_threads() #110884

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
Merged
merged 12 commits into from
Oct 27, 2023
Merged

gh-110205: Fix asyncio ThreadedChildWatcher._join_threads() #110884

merged 12 commits into from
Oct 27, 2023

Conversation

gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Oct 14, 2023 •
edited by bedevere-app bot
Loading

vstinner and others added 4 commits October 13, 2023 02:30
@vstinner
pythongh-110205: Fix asyncio ThreadedChildWatcher._join_threads()
d414ed2 
ThreadedChildWatcher._join_threads() now clears references to
completed threads.

test_asyncio.utils.TestCase now calls _join_threads() of the watcher,
uses SHORT_TIMEOUT to join a thread, and then raises an exception if
there are still running threads.

Rename also ThreadedChildWatcher threads to add "asyncio-" prefix to
the name.
@vstinner
Don't create a new list
f161daa
@vstinner
Fix typo: self.threads => self._threads
dbbad2d
@gvanrossum
Fix code to clear references to terminated threads
85b79b2
@bedevere-app bedevere-app bot mentioned this pull request Oct 14, 2023
Closed
@gvanrossum gvanrossum added tests Tests in the Lib/test dir topic-asyncio skip news needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Oct 14, 2023
@gvanrossum gvanrossum force-pushed the asyncio_clear_watcher_threads branch from fd13f23 to 566fb52 Compare October 15, 2023 00:25
@gvanrossum gvanrossum marked this pull request as ready for review October 15, 2023 09:25
@gvanrossum
Copy link
Member Author

Reviewers, please have a look at this comment in the issue for background on what I ended up doing:
#110205 (comment)

@gvanrossum
Copy link
Member Author

@graingert I wonder if you have time to review this?

graingert
graingert approved these changes Oct 15, 2023
View reviewed changes
Copy link
Contributor

@graingert graingert left a comment

Choose a reason for hiding this comment

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

Ah yeah the old _join_threads was always a noop

graingert
graingert reviewed Oct 15, 2023
View reviewed changes
@gvanrossum
Copy link
Member Author

Assuming this passes I will merge it and backport to 3.12 and 3.11.

@gvanrossum gvanrossum merged commit c3bb10c into python:main Oct 27, 2023
@miss-islington-app
Copy link

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

@gvanrossum gvanrossum deleted the asyncio_clear_watcher_threads branch October 27, 2023 22:44
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 27, 2023
@gvanrossum @miss-islington
pythongh-110205: Fix asyncio ThreadedChildWatcher._join_threads() (py…
8d9326d 
…thonGH-110884)

- `ThreadedChildWatcher.close()` is now *officially* a no-op; `_join_threads()` never did anything.
- Threads created by that class are now named `asyncio-waitpid-NNN`.
- `test.test_asyncio.utils.TestCase.close_loop()` now waits for the child watcher's threads, but not forever; if a thread hangs, it raises `RuntimeError`.
(cherry picked from commit c3bb10c)

Co-authored-by: Guido van Rossum <guido@python.org>
@bedevere-app
Copy link

bedevere-app bot commented Oct 27, 2023

GH-111412 is a backport of this pull request to the 3.12 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 27, 2023
@gvanrossum @miss-islington
pythongh-110205: Fix asyncio ThreadedChildWatcher._join_threads() (py…
ba3e29e 
…thonGH-110884)

- `ThreadedChildWatcher.close()` is now *officially* a no-op; `_join_threads()` never did anything.
- Threads created by that class are now named `asyncio-waitpid-NNN`.
- `test.test_asyncio.utils.TestCase.close_loop()` now waits for the child watcher's threads, but not forever; if a thread hangs, it raises `RuntimeError`.
(cherry picked from commit c3bb10c)

Co-authored-by: Guido van Rossum <guido@python.org>
@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Oct 27, 2023
@bedevere-app
Copy link

bedevere-app bot commented Oct 27, 2023

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

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Oct 27, 2023
gvanrossum added a commit that referenced this pull request Oct 27, 2023
@miss-islington @gvanrossum
[3.12] gh-110205: Fix asyncio ThreadedChildWatcher._join_threads() (G…
2398036 
…H-110884) (#111412)

- `ThreadedChildWatcher.close()` is now *officially* a no-op; `_join_threads()` never did anything.
- Threads created by that class are now named `asyncio-waitpid-NNN`.
- `test.test_asyncio.utils.TestCase.close_loop()` now waits for the child watcher's threads, but not forever; if a thread hangs, it raises `RuntimeError`.
(cherry picked from commit c3bb10c)

Co-authored-by: Guido van Rossum <guido@python.org>
gvanrossum added a commit that referenced this pull request Oct 27, 2023
@miss-islington @gvanrossum
[3.11] gh-110205: Fix asyncio ThreadedChildWatcher._join_threads() (G…
1a01ca4 
…H-110884) (#111413)

- `ThreadedChildWatcher.close()` is now *officially* a no-op; `_join_threads()` never did anything.
- Threads created by that class are now named `asyncio-waitpid-NNN`.
- `test.test_asyncio.utils.TestCase.close_loop()` now waits for the child watcher's threads, but not forever; if a thread hangs, it raises `RuntimeError`.
(cherry picked from commit c3bb10c)

Co-authored-by: Guido van Rossum <guido@python.org>
iritkatriel pushed a commit to iritkatriel/cpython that referenced this pull request Oct 29, 2023
@gvanrossum @iritkatriel
pythongh-110205: Fix asyncio ThreadedChildWatcher._join_threads() (py…
2e74589 
…thon#110884)

- `ThreadedChildWatcher.close()` is now *officially* a no-op; `_join_threads()` never did anything.
- Threads created by that class are now named `asyncio-waitpid-NNN`.
- `test.test_asyncio.utils.TestCase.close_loop()` now waits for the child watcher's threads, but not forever; if a thread hangs, it raises `RuntimeError`.
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@gvanrossum @aisk
pythongh-110205: Fix asyncio ThreadedChildWatcher._join_threads() (py…
f540277 
…thon#110884)

- `ThreadedChildWatcher.close()` is now *officially* a no-op; `_join_threads()` never did anything.
- Threads created by that class are now named `asyncio-waitpid-NNN`.
- `test.test_asyncio.utils.TestCase.close_loop()` now waits for the child watcher's threads, but not forever; if a thread hangs, it raises `RuntimeError`.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@gvanrossum @Glyphack
pythongh-110205: Fix asyncio ThreadedChildWatcher._join_threads() (py…
ecd1749 
…thon#110884)

- `ThreadedChildWatcher.close()` is now *officially* a no-op; `_join_threads()` never did anything.
- Threads created by that class are now named `asyncio-waitpid-NNN`.
- `test.test_asyncio.utils.TestCase.close_loop()` now waits for the child watcher's threads, but not forever; if a thread hangs, it raises `RuntimeError`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@graingert graingert graingert approved these changes

@willingc willingc willingc 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

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

Assignees
No one assigned
Labels
skip news tests Tests in the Lib/test dir topic-asyncio
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gvanrossum @graingert @willingc @vstinner