[issue38356] test_asyncio: SubprocessThreadedWatcherTests leaks threads

[Kyle Stanley]

Kyle Stanley <aeros167 at gmail.com> added the comment:

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.

----------

_______________________________________
Python tracker <report at bugs.python.org>
<https://bugs.python.org/issue38356>
_______________________________________