classification
Title: test_asyncio: SubprocessThreadedWatcherTests leaks threads
Type: Stage: resolved
Components: asyncio, Tests Versions: Python 3.9, Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: aeros, asvetlov, miss-islington, pablogsal, vstinner, yselivanov
Priority: normal Keywords: patch

Created on 2019-10-02 22:53 by vstinner, last changed 2020-01-12 20:29 by aeros. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 16552 merged aeros, 2019-10-03 03:55
PR 17963 merged miss-islington, 2020-01-12 11:03
Messages (8)
msg353784 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-10-02 22:53
Warning seen o AMD64 Ubuntu Shared 3.x buildbot:
https://buildbot.python.org/all/#/builders/141/builds/2593

test_devnull_output (test.test_a=syncio.test_subprocess.SubprocessThreadedWatcherTests) ...
Warning -- threading_cleanup() failed to cleanup 1 threads (count: 1, dangling: 2)

The ThreadedChildWatcher class of asyncio.unix_events doesn't seem to have a method to join all threads. It should be done in tests to prevent "leaking" threads which can have side effects on following tests.
msg353792 - (view) Author: Kyle Stanley (aeros) * (Python committer) Date: 2019-10-03 00:12
I can try to work on fixing this.
msg353808 - (view) Author: Kyle Stanley (aeros) * (Python committer) Date: 2019-10-03 03:06
First I'll work on adding a new method. Here's a few potential names, ordered roughly by my preferences:

1) join_threads()

2) shutdown_threads()

3) shutdown_threadpool()

The first and second options are roughly equal, but I think join_threads() is a bit more specific to what this is doing.

The third option is because a group of threads could be considered a "threadpool", but ThreadedChildWatcher doesn't utilize a dedicated threadpool class of any form, it just uses an internal dict named `_threads` that maps process IDs to threads. So this name might be potentially misleading.

I'm also wondering whether or not this method should have a timeout parameter that defaults to None. I'm thinking we can probably leave it out for now, but I wanted to hear some other opinions on this.

For now, I'll be implementing this as a regular method, but I could make it a coroutine if that's desired. Admittedly, I don't enough have knowledge of the realistic use cases for ThreadedChildWatcher to know if it would be particularly useful to have an awaitable method to join the threads.

Another consideration is if we want this method to join the threads to be called in `ThreadedChildWatcher.close()`. I think it makes sense from a design perspective to join all of the threads when the close method is used. 

Plus, it would allow the method to be integrated with the tests better. Currently, the test uses the same `setUp()` and `tearDown()` for each of the different subprocess watchers. If it weren't called in the `close()` method, we'd have to add an `if isinstance(watcher, ThreadedChildWatcher)` conditional in `tearDown()`. So, I would prefer to have the method called from `close()`.

Finally, should this method be part of the public API, or just a private method? As mentioned earlier, I'm not overly aware of the realistic use cases for ThreadedChildWatcher. As a result, I don't know if it would be especially useful for users to call this method independently, especially if this were called from `close()` as I was suggesting earlier. 

After we reach a consensus on the implementation details and API design for the new method, I'll work on adding an entry in the documentation (if it should be public) and updating test_subprocess.SubprocessThreadedWatcherTests to utilize it.
msg353809 - (view) Author: Kyle Stanley (aeros) * (Python committer) Date: 2019-10-03 03:19
> Another consideration is if we want this method to join the threads to be called in `ThreadedChildWatcher.close()`.

An additional benefit of having the method called from `close()` is that it means we don't have to modify the tests directly. Also, ThreadedChildWatcher's implementation of `close()` does nothing at the moment.
msg359839 - (view) Author: miss-islington (miss-islington) Date: 2020-01-12 11:03
New changeset 0ca7cc7fc0518c24dc9b78c38418e6064e64f148 by Miss Islington (bot) (Kyle Stanley) in branch 'master':
bpo-38356: Fix ThreadedChildWatcher thread leak in test_asyncio (GH-16552)
https://github.com/python/cpython/commit/0ca7cc7fc0518c24dc9b78c38418e6064e64f148
msg359842 - (view) Author: miss-islington (miss-islington) Date: 2020-01-12 11:21
New changeset 33dd75a28fe2ec6e85c5d3b315b5a9d4cf0652db by Miss Islington (bot) in branch '3.8':
bpo-38356: Fix ThreadedChildWatcher thread leak in test_asyncio (GH-16552)
https://github.com/python/cpython/commit/33dd75a28fe2ec6e85c5d3b315b5a9d4cf0652db
msg359843 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2020-01-12 11:22
I hope it is fixed now.
Thanks, Kyle!
msg359868 - (view) Author: Kyle Stanley (aeros) * (Python committer) Date: 2020-01-12 20:29
> I hope it is fixed now.
> Thanks, Kyle!

No problem, thanks for looking over it. 

Let me know if the warning comes up again. If it does, I'll be sure to look into it.
History
Date User Action Args
2020-01-12 20:29:49aerossetmessages: + msg359868
2020-01-12 11:22:37asvetlovsetversions: + Python 3.8
2020-01-12 11:22:31asvetlovsetstatus: open -> closed
resolution: fixed
messages: + msg359843

stage: patch review -> resolved
2020-01-12 11:21:03miss-islingtonsetmessages: + msg359842
2020-01-12 11:03:32miss-islingtonsetpull_requests: + pull_request17372
2020-01-12 11:03:05miss-islingtonsetnosy: + miss-islington
messages: + msg359839
2019-10-03 03:55:02aerossetkeywords: + patch
stage: patch review
pull_requests: + pull_request16140
2019-10-03 03:19:00aerossetmessages: + msg353809
2019-10-03 03:06:28aerossetmessages: + msg353808
2019-10-03 00:12:54aerossetnosy: + aeros
messages: + msg353792
2019-10-02 22:53:10vstinnersetnosy: + pablogsal
2019-10-02 22:53:01vstinnercreate