Skip to content

Conversation

@vstinner
Copy link
Member

@vstinnervstinner commented Oct 13, 2023

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.

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.
Copy link
Member

@gvanrossumgvanrossum left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines 1384 to 1385
self.threads = [thread for thread in list(self._threads.values())
if thread.is_alive() and not thread.daemon]
Copy link
Member

Choose a reason for hiding this comment

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

Why are you creating a new instance variable self.threads here? Did you intend to clean out completed threads from the dict self._threads?

Copy link
MemberAuthor

@vstinnervstinnerOct 13, 2023

Choose a reason for hiding this comment

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

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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Do you mean that you prefer to not create a new list?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I modified my PR to use self.threads[:] = ..., so the self.threads list object is not replacd.

Copy link
Member

Choose a reason for hiding this comment

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

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.

@vstinner
Copy link
MemberAuthor

Is there any advantage to using the internal API _join_threads() instead of the internal variable _threads?

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.

@gvanrossum
Copy link
Member

Please take a break and read the comments and the code. There is no variable self.threads.

@vstinner
Copy link
MemberAuthor

Please take a break and read the comments and the code. There is no variable self.threads.

Ah, I misunderstood your comment. I fixed the typo, the code now sets self._threads.

@vstinnervstinner deleted the asyncio_clear_watcher_threads branch October 13, 2023 17:53
Comment on lines +1384 to +1385
self._threads[:] = [thread for thread in list(self._threads.values())
if thread.is_alive() and not thread.daemon]
Copy link
Member

Choose a reason for hiding this comment

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

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.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting core reviewneeds backport to 3.11only security fixesneeds backport to 3.12only security fixesskip newstestsTests in the Lib/test dirtopic-asyncio

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@vstinner@gvanrossum