bpo-34037: Fix test_asyncio failure and add loop.shutdown_default_executor()

[aeros]

This PR was based heavily upon @vstinner's PR and the suggestions from @asvetlov to improve upon it:

I support the idea but have a question about implementation.
asyncio provides an interface class for loop called AbstractEventLoop.
I believe that the new property should be reflected there.
The next question is: should it be a property or get_wait_executor_on_close() / set_wait_executor_on_close() pair of methods? Asyncio loop is designed to don't use properties but getters / setters.

@vstinner's PR was reverted due to it making too drastic of an API change for 3.8 and the concerns from @asvetlov about adding the property without providing public getter and setter methods.

However, the changes proposed still had significant support, and most importantly, it fixes a bug which has been causing intermittent failures within the CI tests for test_asyncio due to dangling threads:

test_run_in_executor_cancel (test.test_asyncio.test_events.KqueueEventLoopTests) ...

  Warning -- threading_cleanup() failed to cleanup 1 threads (count: 1, dangling: 2)
  Dangling thread: <Thread(ThreadPoolExecutor-13_0, started daemon 34471091200)>
  Dangling thread: <_MainThread(MainThread, started 34393318400)>

Since this is involving changes to the public API, I fully plan on adding the appropriate changes to the documentation. But, I would prefer to wait until I receive feedback from @vstinner and @asvetlov about the implementation itself before doing so since they were both significantly involved in @vstinner's previous PR.

I'll cc the asyncio team as well in case anyone else has feedback to provide.

/cc @python/asyncio

https://bugs.python.org/issue34037

[aeros]

I'll fix the number of characters per line and add the appropriate Sphinx roles after I finish adding the documentation changes.

[aeros]

Adding a DO-NOT-MERGE label until I'm finished with the documentation changes. This PR should not be merged in it's current state, as it only includes the implementation at the moment.

[vstinner]

You have to update the documentation:

• Document the 2 new event loop methods, around https://docs.python.org/dev/library/asyncio-eventloop.html
• Document the behavior change in What's New in Python 3.9, around: https://docs.python.org/dev/whatsnew/3.9.html#changes-in-the-python-api

[bedevere-bot]

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.

[aeros]

Thanks for the feedback @vstinner.

You have to update the documentation

Yep, I was definitely planning on doing so within this PR. I just wanted to make sure the implementation details were satisfactory first, so that I wouldn't have to add the documentation changes and then completely rewrite them from different specifications. I'll start working on that next.

I appreciate that you specified the locations. I figured the documentation for the methods would be on asyncio event loop page, but I wasn't certain where to place the changes in What's New (or whether that should be done within this PR). This will be my first time adding an entry in there, as the majority of my contributions thus far have been PR reviews or documentation changes.

I have to admit, adding my own What's New entry for 3.9 is both a little intimidating and exciting. (:

[1st1]

I still don't like this API and I'd suggest to implement it the other way around. We have a very similar problem with asynchronous generators -- closing them isn't an easy process and can be a lengthy blocking operation. The way we do that is via the loop.shutdown_asyncgens() method, that users are supposed to call manually before loop.close(). This is a low-level API, and the expectation is that asyncio.run() function would do that for you.

Therefore I propose the following:

• Add a new loop.shutdown_threadpool() method. Just like with shutdown_asyncgens() -- it would be invalid to call loop.run_in_executer() after loop.shutdown_threadpool() is called.

• Make asyncio.run() to call the new loop.shutdown_threadpool().

[1st1]

@aeros167 Kyle, do you have time to work on this issue this week?

[aeros]

@1st1:

@aeros167 Kyle, do you have time to work on this issue this week?

Yeah, I definitely should have time to work on it. Would you recommend making the changes directly to this PR or opening a separate one?

I think your proposal would be far more robust and provide a significantly better user experience. But this change might be a decent fallback, mainly for the purpose of fixing the CI tests in case there are unforeseen complications with properly implementing the proposed loop.shutdown_threadpool(). The intermittent failures for test_asyncio are quite problematic.

I would be glad to help with working on implementing it though. I've been interested in becoming more involved with improving the CI tests and asyncio development in general, and I think this process will provide a great introduction to both. Thanks for the in-depth feedback.

[asvetlov]

I support @1st1 proposal: loop.shutdown_threadpool() is more robust than a flag (plus we have the similar shutdown_asyncgens() already.

[1st1]

But this change might be a decent fallback, mainly for the purpose of fixing the CI tests in case there are unforeseen complications with properly implementing the proposed loop.shutdown_threadpool(). The intermittent failures for test_asyncio are quite problematic.

Well, it's a bit too late to add new APIs in the 3.8 branch, no matter if they are public or private. I suggest to go ahead with the new idea so that we can fix the CI for the master branch.

FWIW @vstinner suggested the shutdown_default_executor() name in the bpo, which I think is a better name indeed.

[aeros]

@1st1

My current plan:

1. Add boolean flag self._executor_shutdown_called to BaseEventLoop. This would be used in loop.run_in_executer() to check whether or not the executor shutdown has been called.

2. Create method loop.shutdown_default_executor() (both for AbstractEventLoop and BaseEventLoop). Implementation in BaseEventLoop:

def shutdown_default_executor(self, wait=True):
    # Used by loop.run_in_executer() to check if the the executor has been terminated
    self._executor_shutdown_called = True 
    executor = self._default_executor
    # Nothing to shut down if there is no executor
    if executor is not None: 
        self._default_executor = None
        executor.shutdown(wait) # alternatively, wait=wait, but that seems redundant

3. Modify loop.run_in_executer() to raise a RuntimeError if self._executor_shutdown_called is True. This would be done through an internal loop._check_executor_shutdown() method, called within loop.run_in_executer(), similar to the existing loop._check_closed. Implementation:

def _check_executor_shutdown(self):
    if self._executor_shutdown_called:
        raise RuntimeError("Executor has been shut down")

4. Modify asyncio.run() to use loop._check_executor_shutdown(). This is the part that I'm somewhat unclear about. Since asyncio.run() is already calling loop.close(), can we just modify loop.close() to call the new loop.shutdown_default_executor()? Implementation:

def close(self):
    ...
    self._closed = True
    self._ready.clear()
    self._scheduled.clear()
#    executor = self._default_executor
#    if executor is not None:
#        self._default_executor = None
#        executor.shutdown(wait=self._wait_executor_on_close)
    self.shutdown_default_executor() # default is True

In the above implementation, users could directly utilize loop.shutdown_default_executor() instead of loop.close() if they don't want to wait for executor to finish joining threads before shutting down. From my understanding, wait=True fits the majority of use cases and would be the correct choice to use in asyncio.run().

If further customization is required (or there's issues with the tests timing out in the future), we could add an optional argument thread_timeout to executor.shutdown. If wait is True, this would assign a maximum time to live for each of the threads joined in executor._threads.

5. Update the documentation. Similar to before, I would like to do this step last so that I don't have to rewrite the documentation changes if a different implementation is desired.

If the above is satisfactory, +1 this comment, otherwise let me know if anything is incorrect.

Note: Several of the comments in the examples above are for the purpose of explaining my thought process, and would not be included in the PR. Particularly the inline comments.

[aeros]

I'll change the PR title as well after I commit the changes to the PR branch. I've got some college work to do for now, but I should be able to work on that later tonight or tomorrow.

[asvetlov]

1. Assign self._executor_shutdown_called = True unconditionally.
2. Don't call shutdown_default_executor() from close(); left it for user as we do for shutdown_asyncgens()

[aeros]

@asvetlov:

Assign self._executor_shutdown_called = True unconditionally.

That would make more sense, especially since this is done with self._asyncgens_shutdown_called. The status of the flag should change on call to the method rather than only when there's a default executor set.

Don't call shutdown_default_executor() from close(); left it for user as we do for shutdown_asyncgens()

Hmm, okay. It just seemed odd to me to leave in the part in the last 4 lines of close(), since shutdown_default_executor() is effectively a more robust version of that. Should those lines be removed from close() or remain as is?

I'm not certain where the best place to call shutdown_default_executor() would be from within asyncio.run(). Perhaps like this (line 48 of https://github.com/python/cpython/blob/master/Lib/asyncio/runners.py)?

    ...
    loop = events.new_event_loop()
    try:
        events.set_event_loop(loop)
        loop.set_debug(debug)
        return loop.run_until_complete(main)
    finally:
        try:
            _cancel_all_tasks(loop)
            loop.run_until_complete(loop.shutdown_asyncgens())
            loop.shutdown_default_executor()
        finally:
            events.set_event_loop(None)
            loop.close()

Thanks for the feedback, and I greatly appreciate that you guys have allowed me to help with working on this issue. I've been learning a lot about the internals of asyncio during this process. (:

[asvetlov]

I think your last comment is correct.
Would be nice to get @1st1 feedback as well

[aeros]

Updating the title to better reflect the changes and purpose of the PR. I'll update the PR itself after receiving approval from Yury on the implementation details.

[aeros]

I just realized that I forgot to set self._default_executor = None within my example for shutdown_default_executor. I'll update it accordingly and include Andrew's suggestion regarding the flag as well.

[1st1]

Hmm, okay. It just seemed odd to me to leave in the part in the last 4 lines of close(), since shutdown_default_executor() is effectively a more robust version of that. Should those lines be removed from close() or remain as is?

They should remain as-is.

The reasoning:

1. We must not change the behaviour of the loop.close() method.

2. Right now loop.close() shuts down the executor without waiting; keep it that way.

3. The new loop.shutdown_default_executor() should always wait. Drop the wait parameter. The reason for this function is to let users explicitly shut executor down and wait until it's shutdown.

The asyncio.run() function should call loop.shutdown_default_executor() after loop.shutdown_asyncgens().

Also, I'd make loop.shutdown_default_executor() a coroutine, even though we don't need that now. That would give us room to add additional functionality that might involve awaiting on something, and will make the new function symmetric with loop.shutdown_asyncgens().

[asvetlov]

Also, I'd make loop.shutdown_default_executor() a coroutine, even though we don't need that now. That would give us room to add additional functionality that might involve awaiting on something, and will make the new function symmetric with loop.shutdown_asyncgens().

@1st1 I understand the motivation but curious how it can be done in practice. Waiting is a synchronous call.
loop.shutdown_default_executor() can spawn a thread and wait a future that is set from the thread. Doesn't it look overcomplicated?

[1st1]

loop.shutdown_default_executor() can spawn a thread and wait a future that is set from the thread. Doesn't it look overcomplicated?

I'd just keep it simple and let the coroutine block. We can implement the "wait in a thread" solution later if we need to. Do you think this would be a bad idea?

[aeros]

Oh crap, I just messed that up. I was attempting to update only the master branch in my fork, I did not mean to affect this PR branch. I'll try to revert this branch back to dbc08ae. Apologies for the spam messages, I'm still fairly new to using Git. I must have accidentally left it in the bpo-34037 branch when I made the changes instead of switching to master...

[aeros]

Fixed, I successfully reverted the PR branch to dbc08ae. I would appreciate it if someone could remove the additional reviewers added to the PR, I don't have permission to do that. Sorry about the mess.

[asvetlov]

done

[aeros]

Normally, I haven't left the "Patch by " bit in the news entries for my own PRs, but since this one involved a significant amount of effort on my part, I'd like to do so. I'll update the news entry accordingly.

[asvetlov]

Let's merge it, there is no reason for waiting

[aeros]

@asvetlov:

Let's merge it, there is no reason for waiting

The only reason to wait was getting feedback from @1st1 and @vstinner on whether or not the "What's New" entry should be added, but that can be done at a later point if it's useful/necessary. All of the actual changes were finalized. Thanks for merging it!

[vstinner]

The only reason to wait was getting feedback from @1st1 and @vstinner on whether or not the "What's New" entry should be added, but that can be done at a later point if it's useful/necessary. All of the actual changes were finalized. Thanks for merging it!

New features deserve an entry in the What's New (In Python 3.8) doc, yes. You can open a new PR for that.