GeezerVison

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

GeezerVison is showing github.com — gh-110205: Fix asyncio ThreadedChildWatcher._join_threads() by vstinner · Pull Request #110790 · python/cpython

Conversation

vstinner commented Oct 13, 2023 •

edited by bedevere-app Bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

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.

Issue: test_asyncio.test_subprocess leaked dangling threads on PPC64LE Fedora Stable 3.x: #110205

Sorry, something went wrong.

Uh oh!

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

All reactions


          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 added tests

Tests in the Lib/test dir

topic-asyncio needs backport to 3.11

only security fixes

needs backport to 3.12 only security fixes labels

Oct 13, 2023

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

October 13, 2023 00:30

bedevere-app Bot added the awaiting core review label

Oct 13, 2023

bedevere-app Bot mentioned this pull request

Oct 13, 2023

test_asyncio.test_subprocess leaked dangling threads on PPC64LE Fedora Stable 3.x: #110205

Closed

vstinner added the skip news label

Oct 13, 2023

gvanrossum reviewed

Oct 13, 2023

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.

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.

Uh oh!

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

All reactions

Comment thread

Lib/asyncio/unix_events.py Outdated

Comment on lines +1384 to +1385

		self.threads = [thread for thread in list(self._threads.values())
		if thread.is_alive() and not thread.daemon]

gvanrossum Oct 13, 2023

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.

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.

Uh oh!

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

All reactions

vstinner Oct 13, 2023 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member Author

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.

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.

Uh oh!

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

All reactions

vstinner Oct 13, 2023

Copy link

Copy Markdown

Member Author

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner Oct 13, 2023

Copy link

Copy Markdown

Member Author

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

gvanrossum Oct 13, 2023

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.

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.

Uh oh!

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

All reactions

vstinner commented Oct 13, 2023

Copy link

Copy Markdown

Member Author

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 reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

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


          Don't create a new list

f161daa

gvanrossum commented Oct 13, 2023

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

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


          Fix typo: self.threads => self._threads

dbbad2d

vstinner commented Oct 13, 2023

Copy link

Copy Markdown

Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner closed this

Oct 13, 2023

vstinner deleted the asyncio_clear_watcher_threads branch

October 13, 2023 17:53

gvanrossum reviewed

Oct 13, 2023

View reviewed changes

Comment thread

Lib/asyncio/unix_events.py

Comment on lines +1384 to +1385

		self._threads[:] = [thread for thread in list(self._threads.values())
		if thread.is_alive() and not thread.daemon]

gvanrossum Oct 13, 2023

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.

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.

Uh oh!

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

All reactions

gvanrossum mentioned this pull request

Oct 14, 2023

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

Merged

Reviewers

gvanrossum

gvanrossum left review comments

1st1

Awaiting requested review from 1st1

1st1 is a code owner

asvetlov

Awaiting requested review from asvetlov

asvetlov is a code owner

kumaraditya303

Awaiting requested review from kumaraditya303

kumaraditya303 is a code owner

willingc

Awaiting requested review from willingc

willingc is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

awaiting core review needs backport to 3.11

only security fixes

needs backport to 3.12

only security fixes

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.

Uh oh!

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

2 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

vstinner commented Oct 13, 2023 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

gvanrossum Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

gvanrossum Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner commented Oct 13, 2023

Uh oh!

gvanrossum commented Oct 13, 2023

Uh oh!

vstinner commented Oct 13, 2023

Uh oh!

gvanrossum Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vstinner commented Oct 13, 2023 •

edited by bedevere-app Bot

Loading

Uh oh!

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

vstinner Oct 13, 2023 •

edited

Loading

Uh oh!

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