Skip to content

bpo-38356: Fix ThreadedChildWatcher thread leak in test_asyncio #16552

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

Merged
merged 5 commits into from
Jan 12, 2020
Merged

bpo-38356: Fix ThreadedChildWatcher thread leak in test_asyncio #16552

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4a8395a
Fix test_asyncio.test_subprocess dangling threads
aeros Oct 3, 2019
bfc0d6c
Remove empty line and fix formatting
aeros Oct 3, 2019
fbf7f0d
Fix code formatting
aeros Oct 24, 2019
6bcf009
Don't join daemon threads
aeros Oct 24, 2019
fe2b256
Add docstring for ThreadedChildWatcher._join_threads()
aeros Oct 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion Lib/asyncio/unix_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1266,7 +1266,14 @@ def is_active(self):
return True

def close(self):
pass
self._join_threads()

def _join_threads(self):
"""Internal: Join all non-daemon threads"""
threads = [thread for thread in list(self._threads.values())
if thread.is_alive() and not thread.daemon]
for thread in threads:
thread.join()
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to remove the thread from self._threads as soon as a thread complete?

for pid in list(self._threads):
  thread = self._threads[pid]
  thread.join()
  self._threads.pop(pid)

Would it be possible to log a warning if a thread runs longer than timeout seconds? Or even exit silently if the timoeut is exceeded?

Maybe we should also modify add_child_handler() to raise an exception if it's called after close() has been called? For example, add a _closed attribute? It would prevent to spawn new threads while we wait for existing threads.

Copy link
Contributor Author

@aeros aeros Oct 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@vstinner

Would it be possible to remove the thread from self._threads as soon as a thread complete?

Yeah sure, I would have no issue with adding something like this. I'm not 100% sure that we have to remove the threads that aren't alive from self._threads, but it would certainly be possible.

What exactly does removing the threads from self._threads accomplish that joining them doesn't already do? When a thread is successfully joined, thread._is_stopped is set to true, which is what thread.is_alive() uses to check if the thread is still active. I don't think we necessarily need to remove the threads from self._threads as part of the cleanup. Unless there's something I'm missing, I think that would just add additional overhead.

Would it be possible to log a warning if a thread runs longer than timeout seconds? Or even exit silently if the timoeut is exceeded?

This should be possible by running thread.join(timeout) and if thread.is_alive() is True afterwards, the join timed out. In what situations would you want to differentiate between logging a warning vs silently exiting?

Unless by "exit silently" you were referring to thread.join(), which effectively does that already when the timeout is reached. thread.join(timeout) will block for timeout duration or when the join is complete, whichever one occurs first.

Maybe we should also modify add_child_handler() to raise an exception if it's called after close() has been called? For example, add a _closed attribute? It would prevent to spawn new threads while we wait for existing threads.

Yeah this would be a fairly straightforward change and I agree with it. A RuntimeError would be suitable as an exception to raise for that scenario.


def __enter__(self):
return self
Expand Down