A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

@asvetlov

As @vstinner mentioned watcher.close() is not called normally.
Moreover, the watcher is designed to work with multiple event loops.

This was primarily based upon fixing the thread leak in the unittest SubprocessThreadedWatcherTests that uses SubprocessWatcherMixin, which does use watcher.close():

    class SubprocessWatcherMixin(SubprocessMixin):
        Watcher = None
        def setUp(self):
            super().setUp()
            policy = asyncio.get_event_loop_policy()
            self.loop = policy.new_event_loop()
            self.set_event_loop(self.loop)
            watcher = self.Watcher()
            watcher.attach_loop(self.loop)
            policy.set_child_watcher(watcher)

        def tearDown(self):
            super().tearDown()
            policy = asyncio.get_event_loop_policy()
            watcher = policy.get_child_watcher()
            policy.set_child_watcher(None)
            watcher.attach_loop(None)
            watcher.close()

I figured the easiest way to have the fix conform to the existing tests would be to have all of the threads joined in ThreadedChildWatcher.close(). Is there a different solution you would recommend that would fix the leak coming from the ThreadedChildWatcher's threads? The only other option I thought of was changing the tearDown():

        def tearDown(self):
            super().tearDown()
            policy = asyncio.get_event_loop_policy()
            watcher = policy.get_child_watcher()
            policy.set_child_watcher(None)
            watcher.attach_loop(None)
            if isinstance(watcher, ThreadedChildWatcher):
                watcher._join_threads()
            watcher.close()

But, this would work even worse in terms to compatibility with other implementations, and adds an unneeded conditional to the other watcher class tests.

We should clean up a watcher after this step (again, explicitly).

Do you have an implementation idea as to how this could be done? Would each watcher have it's own cleanup method separate from watcher.close()?

@vstinner

Can we imagine to add a new method to only wait for threads spawned by an event loop?

Would this still fix the dangling thread issue in the SubprocessThreadedWatcherTests? As far as I can tell, only the watcher itself is closed, not the event loop.

@asvetlov

I disagree with the PR.

After reading through your comment, I also disagree with the current state of it. As I mentioned in the bpo comments, I really didn't have an idea of the intended public API design for ThreadedChildWatcher (beyond what is said in the docs), this was mostly just an attempt to fix the dangling thread issue.

I'd be more than glad to modify this PR though in any recommended manner that you think would fit the intended API design.

@asvetlov

We should clean up a watcher after this step (again, explicitly).

Also, there's something else that's not clear to me about this. In the current docs, it says the watcher.close() method is intended to be used as a resource cleanup (https://docs.python.org/3.9/library/asyncio-policy.html#asyncio.AbstractChildWatcher.close):

close()

    Close the watcher.

    This method has to be called to ensure that underlying resources are cleaned-up.

So where else other than watcher.close() (which the current tests are using) should the watcher be cleaned up from?

Well, joining spawn threads in ThreadedChildWatcher.close() makes sense.
asyncio doesn't call it on policy finishing, thus tearDown() should call watcher.close() as @aeros167 suggested.

@asvetlov

thus tearDown() should call watcher.close() as @aeros167 suggested.

Just to make sure it's clear, tearDown() already calls watcher.close(). The code example in my prior comment was the existing implementation.

So if we're in agreement about joining the threads in watcher.close() and retaining the current tearDown() implementation where watcher.close() is already called at the end, what do you think should be changed from the current version of the PR? I'm curious to hear @1st1's thoughts on this as well.

Looks OK to me. If @asvetlov approves this it's good to go.

Looks OK to me. If @asvetlov approves this it's good to go.

Okay, I'll just add the docstring then after I finish working on the 3.8 asyncio "What's New" changes.

In the recently added commits, I fixed the formatting, added a docstring as requested by @1st1, and made a slight modification to not join daemon threads in _join_threads().

This is because daemon threads don't need to be joined as part of this cleanup process. When the program exits, they're terminated automatically. See #16552 (comment) for more details.

@asvetlov Are there any changes you'd like to suggest for this PR or is it good to go?

Thanks @aeros for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

GH-17963 is a backport of this pull request to the 3.8 branch.