Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 27 commits into from Sep 19, 2019

Conversation

@aeros167
Copy link
Member

commented Sep 9, 2019

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

@@ -0,0 +1 @@
For :mod:`asyncio`, add attribute ``asyncio.BaseEventLoop.wait_executor_on_close`` to provide an option for waiting for the executor before closing the event loop. Also adds methods ``loop.get_wait_executor_on_close`` and ``loop.set_wait_executor_on_close``.

This comment has been minimized.

Copy link
@aeros167

aeros167 Sep 9, 2019

Author Member

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

@aeros167

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

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
Copy link
Member

left a comment

You have to update the documentation:

Lib/asyncio/base_events.py Outdated Show resolved Hide resolved
@bedevere-bot

This comment has been minimized.

Copy link

commented Sep 9, 2019

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.

@aeros167

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

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
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

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

@aeros167

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

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

@1st1

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

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.

@aeros167

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

@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
  1. 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")
  1. 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.

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

@aeros167

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

  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()
@aeros167

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

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

@aeros167

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

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.

@aeros167 aeros167 changed the title bpo-34037: Add BaseEventLoop.wait_executor_on_close and a public interface bpo-34037: Fix test_asyncio failure and add loop.shutdown_default_executor() Sep 10, 2019

@aeros167

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

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

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

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

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

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?

@aeros167 aeros167 requested review from pganssle, rhettinger, tiran and python/windows-team as code owners Sep 15, 2019

@aeros167

This comment has been minimized.

Copy link
Member Author

commented Sep 15, 2019

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

@aeros167 aeros167 force-pushed the aeros167:bpo-34037 branch from e664e38 to dbc08ae Sep 15, 2019

@aeros167

This comment has been minimized.

Copy link
Member Author

commented Sep 15, 2019

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 asvetlov removed request for python/windows-team, gpshead, methane, tiran, abalkin, pganssle, rhettinger, gvanrossum and ilevkivskyi Sep 15, 2019

@asvetlov

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2019

done

@aeros167 aeros167 requested a review from vstinner Sep 16, 2019

@aeros167

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2019

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 asvetlov merged commit 9fdc64c into python:master Sep 19, 2019

4 checks passed

Azure Pipelines PR #20190918.38 succeeded
Details
bedevere/issue-number Issue number 34037 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@asvetlov

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

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

@aeros167

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2019

@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!

@aeros167 aeros167 deleted the aeros167:bpo-34037 branch Sep 20, 2019

@vstinner

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

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.

miss-islington added a commit that referenced this pull request Sep 20, 2019
Doc: Remove provisional note for asyncio.run() (GH-16310)
Based on a comment from @asvetlov #15735 (comment), this removes the provisional note for ``asyncio.run()`` in the documentation.

Automerge-Triggered-By: @1st1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.