gh-110205: Fix asyncio ThreadedChildWatcher._join_threads()#110790
gh-110205: Fix asyncio ThreadedChildWatcher._join_threads()#110790vstinner wants to merge 3 commits intopython:mainfrom
Conversation
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
added
tests
vstinner
requested review from
1st1,
asvetlov,
gvanrossum,
kumaraditya303 and
willingc
as code owners
gvanrossum
left a comment
gvanrossum
left a comment
There was a problem hiding this comment.
Is there any advantage to using the internal API _join_threads() instead of the internal variable _threads? You could just add the timeout arg to the join() call in the existing test cleanup code.
Lib/asyncio/unix_events.py
Outdated
| self.threads = [thread for thread in list(self._threads.values()) | ||
| if thread.is_alive() and not thread.daemon] |
There was a problem hiding this comment.
Why are you creating a new instance variable self.threads here? Did you intend to clean out completed threads from the dict self._threads?
There was a problem hiding this comment.
I used the list comprehension above as an example, for me it looks easier to remove completed threads rather than having a regular list and removing items from the list.
The purpose of this line is to remove completed threads to save memory and to fix the dangling threads issue.
There was a problem hiding this comment.
Do you mean that you prefer to not create a new list?
There was a problem hiding this comment.
I modified my PR to use self.threads[:] = ..., so the self.threads list object is not replacd.
There was a problem hiding this comment.
Do you mean that you prefer to not create a new list?
No I meant that the tests fail because self._threads is a dict, not a list.
It fix the issue related to the PR: dangling thread. The warning can be emitted when the Python object still exist, even if the thread is no longer running. I don't think that copy/paste the _join_threads() logic in the test is worth it. |
|
Please take a break and read the comments and the code. There is no variable |
Ah, I misunderstood your comment. I fixed the typo, the code now sets |
| self._threads[:] = [thread for thread in list(self._threads.values()) | ||
| if thread.is_alive() and not thread.daemon] |
There was a problem hiding this comment.
As I was trying to let you see for yourself, self._threads is a dict, not a list. If you want to update it using a comprehension you can use something like this:
self._threads = {key: thread for key, thread in self._threads.items() if thread.daemon or thread.is_alive()}
Please test locally before merging.
2 participants
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.