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

Deprecate get_event_loop() #83710

Closed
asvetlov opened this issue Feb 2, 2020 · 21 comments
Closed

Deprecate get_event_loop() #83710

asvetlov opened this issue Feb 2, 2020 · 21 comments
Labels
3.10 expert-asyncio type-feature

Comments

@asvetlov
Copy link
Contributor

@asvetlov asvetlov commented Feb 2, 2020

BPO 39529
Nosy @asvetlov, @bdarnell, @serhiy-storchaka, @1st1, @minrk, @tirkarthi, @aeros, @johnmellor
PRs
  • #23554
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2021-04-25.10:43:30.491>
    created_at = <Date 2020-02-02.11:11:37.189>
    labels = ['type-feature', '3.10', 'expert-asyncio']
    title = 'Deprecate get_event_loop()'
    updated_at = <Date 2022-02-25.11:20:12.956>
    user = 'https://github.com/asvetlov'

    bugs.python.org fields:

    activity = <Date 2022-02-25.11:20:12.956>
    actor = 'johnmellor'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-04-25.10:43:30.491>
    closer = 'serhiy.storchaka'
    components = ['asyncio']
    creation = <Date 2020-02-02.11:11:37.189>
    creator = 'asvetlov'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 39529
    keywords = ['patch']
    message_count = 21.0
    messages = ['361229', '361245', '361247', '361248', '361249', '361609', '362487', '362674', '381860', '381881', '382044', '391851', '394732', '394821', '407429', '407430', '407432', '407441', '407448', '407453', '407600']
    nosy_count = 8.0
    nosy_names = ['asvetlov', 'Ben.Darnell', 'serhiy.storchaka', 'yselivanov', 'minrk', 'xtreak', 'aeros', 'johnmellor']
    pr_nums = ['23554']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue39529'
    versions = ['Python 3.10']

    @asvetlov
    Copy link
    Contributor Author

    @asvetlov asvetlov commented Feb 2, 2020

    Yuri proposed it for Python 3.8 but at that time the change was premature.
    Now we can reconsider it for 3.9

    The problem is that asyncio.get_event_loop() not only returns a loop but also creates it on-demand if the thread is main and the loop doesn't exist.

    It leads to weird errors when get_event_loop() is called at import-time and asyncio.run() is used for asyncio code execution.

    get_running_loop() is a much better alternative when used *inside* a running loop, run() should be preferred for calling async code at top-level. Low-level new_event_loop()/loop.run_until_complete() are still present to run async code if top-level run() is not suitable for any reason.

    asyncio.run() was introduced in 3.7, deprecation on get_event_loop() in 3.8 was able to complicate support of 3.5/3.6 by third-party libraries.
    3.5 now reached EOL, 3.6 is in the security-fix mode and going close to EOL. Most people are migrated to newer versions already if they care.
    The maintenance burden of the introduced deprecation should be pretty low.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Feb 2, 2020

    Will asyncio.get_event_loop() be removed or made an alias of asyncio.get_running_loop()? The latter could minimize the disruption of the existing code.

    @asvetlov
    Copy link
    Contributor Author

    @asvetlov asvetlov commented Feb 2, 2020

    Currently, I'm talking about adding a deprecation only.
    The asyncio.get_event_loop() function will stay in Python for a while in deprecated status. I don't know the exact period but it should be 3 releases at least I guess, with possibly extending to 5 releases if needed.

    @asvetlov
    Copy link
    Contributor Author

    @asvetlov asvetlov commented Feb 2, 2020

    Serhiy, maybe I had not understood your proposal properly.

    If you are asking about deprecating get_event_loop() call from outside of async code but implicit call get_running_loop() without a warning if called from async function -- it sounds like a brilliant idea.

    Something like:

    def get_event_loop():
        current_loop = _get_running_loop()
        if current_loop is not None:
            return current_loop
        warnings.warn("...", DeprecationWarning)  
        return get_event_loop_policy().get_event_loop()

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Feb 2, 2020

    Yes, it is what I meant.

    Many code written before 3.7 use get_event_loop() because there was no get_running_loop() yet. Now, to avoid deprecation (and making the code more robust) it should be rewritten with using get_running_loop() or get_event_loop() depending on Python version. It is cumbersome, and causes a code churn.

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Feb 7, 2020

    I think we still use get_event_loop() in asyncio code, no? If we do, we should start by raising deprecation warnings in those call sites, e.g. if a Future or Lock is created outside of a coroutine and no explicit event loop is passed. We should do this in 3.9. We can then think about deprecating get_event_loop in 3.10.

    @aeros
    Copy link
    Member

    @aeros aeros commented Feb 23, 2020

    FWIW, I agree with get_event_loop() being problematic with its error messages, and having a bit too much functionality in a single function from an API design perspective. It commonly comes up as an issue among asyncio users in communities that I'm active in.

    Typically, I advise them to use get_running_loop() where possible, as it can be directly substituted within a coroutine func. For __init__ methods, I recommend setting their internal loop attribute to None and then setting it to get_running_loop() in either a coro start() method or coro factory method. Outside of directly managing the event loop, this works for many use cases. When it's needed to actually create one, I advise them to use new_event_loop(), but mention that it's already handled if they're using asyncio.run().

    Andrew Svetlov wrote:

    The asyncio.get_event_loop() function will stay in Python for a while in deprecated status. I don't know the exact period but it should be 3 releases at least I guess, with possibly extending to 5 releases if needed.

    With how many libraries that rely on it, I suspect it will likely be a very slow transition from deprecation to removal. 4 versions seems like a reasonable period to me, but I think that 3 may be too short (assuming we retain the newer annual release cycle).

    Yury Selivanov wrote:

    I think we still use get_event_loop() in asyncio code, no?

    Indeed, we currently use it in several places throughout asyncio. From a brief glace using git grep "get_event_loop()":

    Lib/asyncio/futures.py:76: self._loop = events.get_event_loop()
    Lib/asyncio/futures.py:390: loop = events.get_event_loop()
    Lib/asyncio/locks.py:81: self._loop = events.get_event_loop()
    Lib/asyncio/locks.py:177: self._loop = events.get_event_loop()
    Lib/asyncio/locks.py:244: self._loop = events.get_event_loop()
    Lib/asyncio/locks.py:375: self._loop = events.get_event_loop()
    Lib/asyncio/queues.py:35: self._loop = events.get_event_loop()
    Lib/asyncio/streams.py:45: loop = events.get_event_loop()
    Lib/asyncio/streams.py:82: loop = events.get_event_loop()
    Lib/asyncio/streams.py:104: loop = events.get_event_loop()
    Lib/asyncio/streams.py:120: loop = events.get_event_loop()
    Lib/asyncio/streams.py:147: self._loop = events.get_event_loop()
    Lib/asyncio/streams.py:403: self._loop = events.get_event_loop()
    Lib/asyncio/subprocess.py:206: loop = events.get_event_loop()
    Lib/asyncio/subprocess.py:227: loop = events.get_event_loop()
    Lib/asyncio/tasks.py:69: loop = events.get_event_loop()
    Lib/asyncio/tasks.py:129: loop = events.get_event_loop()
    Lib/asyncio/tasks.py:590: loop = events.get_event_loop()
    Lib/asyncio/tasks.py:669: loop = events.get_event_loop()
    Lib/asyncio/tasks.py:751: loop = events.get_event_loop()

    For brevity, I omitted the docs, tests, and the function definition for get_event_loop().

    Based on Serhiy's idea (of making get_event_loop() an alias for get_running_loop() without warning inside of a coro func, but warning for using it outside of one), many of the above could remain as is to reduce some code churn. We just have to make sure the documentation is updated to reflect get_event_loop() becoming an alias for get_running_loop(), at the same time as the deprecation warning is added for using it outside of a coro func. Otherwise, I suspect it could lead to significant confusion from users that have warnings enabled.

    That being said, I think we should eventually remove asyncio.get_event_loop() entirely from the asyncio internals, including the ones that wouldn't raise deprecation warnings (if/when it's made an alias to get_running_loop()) for improved clarity. Personally, I find that even the name get_event_loop() is rather vague; get_running_loop() is much more obvious as to its purpose and what it does from a readability perspective.

    Yury Selivanov wrote:

    If we do, we should start by raising deprecation warnings in those call sites, e.g. if a Future or Lock is created outside of a coroutine and no explicit event loop is passed.

    For asyncio.Lock (plus other synchronization primitives) and asyncio.Queue, this would be added in #18195. Currently waiting on emanu (author of the PR) to finish up some changes, but it's mostly complete. Could I work on adding it to asyncio.Future and other classes in the meantime?

    One point to be clarified though: you mention "created outside of a coroutine and no explicit event loop is passed". However, there are already several deprecations in place for passing an explicit event loop for most (if not all) of the __init__ methods for objects across asyncio's high-level API. In those cases, should the deprecation for creating the object outside of a coroutine function care about whether or not an explicit event loop is passed?

    I can see why it would matter for the lower level parts of the API (such as asyncio.Future) where passing the event loop explicitly is still allowed, but IMO it shouldn't be a factor for ones where passing the event loop explicitly is already deprecated. Especially considering that the loop argument will be removed from those entirely in 3.10 (according to the version listed in the current deprecation warnings added in 3.8).

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Feb 26, 2020

    For asyncio.Lock (plus other synchronization primitives) and asyncio.Queue, this would be added in #18195. Currently waiting on emanu (author of the PR) to finish up some changes, but it's mostly complete. Could I work on adding it to asyncio.Future and other classes in the meantime?

    I think the approach should be different:

      # leading underscore is significant:
      loop = asyncio._get_running_loop()  
      if loop is None:
        issue_deprecation_warning()
        loop = asyncio.get_event_loop()

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Nov 25, 2020

    I am writing a code (it is not trivial to emit correct warning) and I have a question. What to do if the loop was set by set_event_loop()? Currently get_event_loop() can return a loop without creating a new loop while get_running_loop() raises an error. Should get_event_loop() still support this in future or set_event_loop() will be deprecated?

    @asvetlov
    Copy link
    Contributor Author

    @asvetlov asvetlov commented Nov 26, 2020

    I think the deprecation of set_event_loop() is a good idea.
    The function is not required by asyncio.run() implementation.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Nov 29, 2020

    The most visible effect in tests is that asyncio.gatcher() and some other synchronous functions will need a running loop if any of arguments is a coroutine.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Apr 25, 2021

    New changeset 172c0f2 by Serhiy Storchaka in branch 'master':
    bpo-39529: Deprecate creating new event loop in asyncio.get_event_loop() (GH-23554)
    172c0f2

    @serhiy-storchaka serhiy-storchaka added type-feature 3.10 labels Apr 25, 2021
    @serhiy-storchaka serhiy-storchaka added the type-feature label Apr 25, 2021
    @bdarnell
    Copy link
    Mannequin

    @bdarnell bdarnell mannequin commented May 29, 2021

    The maintenance burden of the introduced deprecation should be pretty low.

    This is going to cause an unpleasant amount of churn in the Tornado community. It's been idiomatic (going back 12 years now) to do all your setup in synchronous code before starting the event loop. I'm tempted to restore the implicit loop creation in Tornado's IOLoop.current() (which is now essentially a wrapper around asyncio.get_event_loop()).

    It leads to weird errors when get_event_loop() is called at import-time and asyncio.run() is used for asyncio code execution.

    Checks asyncio.run's implementation Oh, I can see how that's a problem (and I've been giving people bad advice about the interchangeability of Tornado's IOLoop.run_sync and asyncio.run). But why does asyncio.run unconditionally create a new event loop instead of running on asyncio.get_event_loop? If asyncio.run used asyncio.get_event_loop it seems to me that we'd have the expected behavior and wouldn't have to make breaking changes to get_event_loop.

    Low-level new_event_loop()/loop.run_until_complete() are still present to run async code if top-level run() is not suitable for any reason.

    What if you're using run_forever instead of run_until_complete? (this is the common mode of operation for Tornado) There are solutions, of course (my preferred one is await asyncio.Event().wait()), but it's an extra conceptual hurdle to throw at users in a "hello world" example and this is why I've stuck with the model that uses (the equivalent of) run_forever instead of asyncio.run.

    @aeros
    Copy link
    Member

    @aeros aeros commented May 31, 2021

    But why does asyncio.run unconditionally create a new event loop instead of running on asyncio.get_event_loop?

    AFAIK, it does so for purposes of compatibility in programs that need multiple separate event loops and providing a degree of self-dependency. asyncio.run() is entirely self-reliant in that it creates all needed resources at the start and closes them in finalization, rather than depending on existing resources. I believe this to be significantly safer and better guaranteed to function as intended, although perhaps at some cost to convenience in cases like your own where there only needs to be one event loop.

    @minrk
    Copy link
    Mannequin

    @minrk minrk mannequin commented Dec 1, 2021

    The comments in this thread suggest that set_event_loop should also be deprecated, but it hasn't been. It doesn't seem to have any use without get_event_loop().

    I'm trying to understand the consequences of these changes for IPython, and make the changes intended by asyncio folks, but am not quite clear, yet.

    If I understand it correctly, this means that the whole concept of a 'current' event loop is deprecated while no event loop is running?

    My interpretation of these changes is that it means any persistent handles on any event loop while it isn't running is fully the responsibility of individual libraries (e.g. tornado, IPython).

    This is coming up in IPython where we need a handle on the event loop and advance it with run_until_complete for each iteration (it should be the same loop to maintain persistent state across advances, so asyncio.run() would not be appropriate). We previously relied on get_event_loop to manage this handle, but I think we have to now shift to tracking our own handle, and can no longer rely on standard APIs to track a shared instance across packages.

    @minrk
    Copy link
    Mannequin

    @minrk minrk mannequin commented Dec 1, 2021

    Further digging reveals that policy.get_event_loop() is not deprecated while asyncio.get_event_loop() is. Is that intentional? Does that mean switching our calls to get_event_loop_policy().get_event_loop() should continue to work without deprecation?

    @asvetlov
    Copy link
    Contributor Author

    @asvetlov asvetlov commented Dec 1, 2021

    IMHO, asyncio.set_event_loop() and policy.get_event_loop()/policy.set_event_loop() are not deprecated by oversight.

    In IPython, I think you could use new_event_loop() for getting a new loop instance.
    Then, save the loop reference somewhere as a direct attribute, threading.local or ContextVar.
    Calling loop.run_until_complete() looks pretty normal in your situation.

    At my job, we have Runner class, the basic usage is:
    with Runner() as runner:
    runner.run(async_func())

    The implementation is pretty close to asyncio.run() but runner.run(...) can be called multiple times. Async context manager interface is responsible for resource closing.

    Maybe I should extract the implementation into a third-party library, I've found this concept useful for CLI applications at least.

    @minrk
    Copy link
    Mannequin

    @minrk minrk mannequin commented Dec 1, 2021

    Thank you! I think I have enough information to update.

    IMHO, asyncio.set_event_loop()...[is] not deprecated by oversight.

    I'm curious, what is an appropriate use of asyncio.set_event_loop() if you can never get the event loop with get_event_loop()? If you always have to pass the handle around anyway, I'm not sure what the use case for a write-only global would be.

    @asvetlov
    Copy link
    Contributor Author

    @asvetlov asvetlov commented Dec 1, 2021

    Ages ago, get_event_loop() did not return the current running loop if called from async function context; it returned a loop instance registered with previous set_event_loop(...) call instead.

    Later we fixed get_event_loop() behavior in 3.5.4 IIRC and added get_running_loop() in 3.7

    set_event_loop() doesn't make sense now from my perspective.

    @minrk
    Copy link
    Mannequin

    @minrk minrk mannequin commented Dec 1, 2021

    Oops, I interpreted "not deprecated by oversight" as the opposite of what you meant. Sorry! All clear, now.

    @bdarnell
    Copy link
    Mannequin

    @bdarnell bdarnell mannequin commented Dec 3, 2021

    In IPython, I think you could use new_event_loop() for getting a new loop instance.
    Then, save the loop reference somewhere as a direct attribute, threading.local or ContextVar.
    Calling loop.run_until_complete() looks pretty normal in your situation.

    That works if you're a leaf in the dependency tree, but what about a library like Tornado (which IPython depends on)? If intermediate libraries (say Tornado and Twisted) introduce their own thread-local loops for backwards compatibility of their existing interfaces, the ecosystem gets fragmented again. Having the thread-local live in asyncio is useful for seamless interoperability across frameworks.

    asyncio.run() is entirely self-reliant in that it creates all needed resources at the start and closes them in finalization, rather than depending on existing resources. I believe this to be significantly safer and better guaranteed to function as intended, although perhaps at some cost to convenience in cases like your own where there only needs to be one event loop.

    Whether or not this is more likely to function as intended depends on assumptions about user expectations. In Tornado (and other async systems I have used in the past), it is the norm to set up handlers on an event loop while it is not running and then start it afterwards. The behavior of asyncio.run is surprising to me. Maybe I'm weird, but regardless of which behavior is surprising to the fewest people, my point is that this change is very disruptive to Tornado and most applications using it, contrary to the claim that the maintenance burden should be pretty low.

    I realize it may be too late to change course, but my preference would have been to resolve the conflict between get_event_loop and asyncio.run by making asyncio.run essentially an alias for asyncio.get_event_loop().run_until_complete. This would relax the isolation of asyncio.run, but that doesn't seem very important to me. The comparable idioms in Tornado (using the IOLoop.run_sync method) all work this way and I've never seen any confusion related to this or a situation in which creating a brand-new event loop in run_sync would be desirable.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    SomberNight added a commit to SomberNight/electrum that referenced this issue Apr 29, 2022
    asyncio.get_event_loop() became deprecated in python3.10. (see python/cpython#83710)
    ```
    .../electrum/electrum/daemon.py:470: DeprecationWarning: There is no current event loop
      self.asyncio_loop = asyncio.get_event_loop()
    .../electrum/electrum/network.py:276: DeprecationWarning: There is no current event loop
      self.asyncio_loop = asyncio.get_event_loop()
    ```
    Also, according to that thread, "set_event_loop() [... is] not deprecated by oversight".
    So, we stop using get_event_loop() and set_event_loop() in our own code.
    Note that libraries we use (such as the stdlib for python <3.10), might call get_event_loop,
    which then relies on us having called set_event_loop e.g. for the GUI thread. To work around
    this, a custom event loop policy providing a get_event_loop implementation is used.
    
    Previously, we have been using a single asyncio event loop, created with
    util.create_and_start_event_loop, and code in many places got a reference to this loop
    using asyncio.get_event_loop().
    Now, we still use a single asyncio event loop, but it is now stored as a global in
    util._asyncio_event_loop (access with util.get_asyncio_loop()).
    
    I believe these changes also fix spesmilo#5376
    SomberNight added a commit to SomberNight/electrum that referenced this issue Apr 29, 2022
    asyncio.get_event_loop() became deprecated in python3.10. (see python/cpython#83710)
    ```
    .../electrum/electrum/daemon.py:470: DeprecationWarning: There is no current event loop
      self.asyncio_loop = asyncio.get_event_loop()
    .../electrum/electrum/network.py:276: DeprecationWarning: There is no current event loop
      self.asyncio_loop = asyncio.get_event_loop()
    ```
    Also, according to that thread, "set_event_loop() [... is] not deprecated by oversight".
    So, we stop using get_event_loop() and set_event_loop() in our own code.
    Note that libraries we use (such as the stdlib for python <3.10), might call get_event_loop,
    which then relies on us having called set_event_loop e.g. for the GUI thread. To work around
    this, a custom event loop policy providing a get_event_loop implementation is used.
    
    Previously, we have been using a single asyncio event loop, created with
    util.create_and_start_event_loop, and code in many places got a reference to this loop
    using asyncio.get_event_loop().
    Now, we still use a single asyncio event loop, but it is now stored as a global in
    util._asyncio_event_loop (access with util.get_asyncio_loop()).
    
    I believe these changes also fix spesmilo#5376
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 expert-asyncio type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants