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.

I can try to work on fixing this.

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.

> 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.

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

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

I hope it is fixed now.
Thanks, Kyle!

> 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.

stage: patch review -> resolved