Repository navigation
gh-110205: Fix asyncio ThreadedChildWatcher._join_threads() - #110790
vstinner wants to merge 3 commits into
Conversation
Sorry, something went wrong.
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.
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.
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Do you mean that you prefer to not create a new list?
Sorry, something went wrong.
There was a problem hiding this comment.
I modified my PR to use self.threads[:] = ..., so the self.threads list object is not replacd.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
Please take a break and read the comments and the code. There is no variable self.threads. |
Sorry, something went wrong.
Ah, I misunderstood your comment. I fixed the typo, the code now sets self._threads. |
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
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.