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

DeprecationWarning scope expanded in asyncio.events #100160

Closed
jaraco opened this issue Dec 10, 2022 · 28 comments
Closed

DeprecationWarning scope expanded in asyncio.events #100160

jaraco opened this issue Dec 10, 2022 · 28 comments
Assignees
Labels
3.10 only security fixes 3.11 only security fixes release-blocker topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@jaraco
Copy link
Member

jaraco commented Dec 10, 2022

As discovered in prompt-toolkit/python-prompt-toolkit#1696, it appears that the DeprecationWarning introduced in Python 3.10 has expanded its scope, now with 3.11.1 and 3.10.9 emitting during get_event_loop_policy() where it did not before:

 ~ $ py -3.11 -W error
Python 3.11.0 (main, Oct 26 2022, 19:06:18) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import asyncio
>>> asyncio.get_event_loop_policy().get_event_loop()
<_UnixSelectorEventLoop running=False closed=False debug=False>
>>> ^D
 ~ $ docker run -it jaraco/multipy-tox py -3.11 -W error
Python 3.11.1 (main, Dec  7 2022, 01:11:34) [GCC 11.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import asyncio
>>> asyncio.get_event_loop_policy().get_event_loop()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.11/asyncio/events.py", line 687, in get_event_loop
    warnings.warn('There is no current event loop',
DeprecationWarning: There is no current event loop

It's not obvious if it was intentional to expand the scope of the DeprecationWarning, but it does come as a surprise as the calling code had previously attempted to address the deprecation.

I think there are two concerns to be addressed:

  • Does this symptom indicate an unintentional but real additional path that was previously unprotected by the DeprecationWarning? And if so, does that imply that the behavior's first true deprecation is in Python 3.12 and thus will delay the removal?
  • What is a user expected to do to properly address the deprecation? I read the what's new for Python 3.10 and it indicates that the call is deprecated, but it provides little guidance on how a user can adapt to the new behavior. Maybe there should be a note to the effect of "programs relying on a non-running event loop must ensure that there is a running event loop before attempting to get the event loop."

Linked PRs

@jaraco jaraco added the type-bug An unexpected behavior, bug, or error label Dec 10, 2022
@gvanrossum
Copy link
Member

@kumaraditya303 @serhiy-storchaka Looks like the 3.12 code somehow made it into 3.11.1 (and 3.10.9). We should roll this back.

the culprit appears to be #93453.

@kumaraditya303
Copy link
Contributor

I am confused, yes there is a change in behavior but it is correct. You need to set the event loop before getting it alternatively use the runner APIs.

I looked at the code and it is missing setting the event loop:
https://github.com/prompt-toolkit/python-prompt-toolkit/blob/da05f669d00817655f76b82972272d4d5f4d4225/src/prompt_toolkit/eventloop/utils.py#L106-L118 The last line gets the event loop without setting it. Am I missing something?

@gvanrossum
Copy link
Member

In pre-3.12 semantics, the first ever get_event_loop() call (if in the main thread) creates a new loop and sets it as the current loop. Apparently those semantics were lost?

@gvanrossum
Copy link
Member

We shouldn't have a change in behavior between 3.11.0 and 3.11.1.

@serhiy-storchaka
Copy link
Member

It was an intentional change based on results of discussion in #93453. BaseDefaultEventLoopPolicy.get_event_loop() will raise an exception in 3.12.

A user is expected to only use get_event_loop() if an event loop was previously set by set_event_loop(), and use new_event_loop() for explicit creation of the event loop. But if you do not know whether an even loop was previously set, the only workaround is silencing DeprecationWarning for the line with get_event_loop().

@gvanrossum
Copy link
Member

I'm guessing it's unfortunate that we have a deprecation warning in 3.11.1 that wasn't there in 3.11.0, and ditto for 3.10.9 vs. 3.10.8. That's not normally how we do deprecations (unless it's security related).

I'd like an opinion from an SC member -- let me tag @gpshead, he can decide whether to escalate this to the SC agenda or whatever.

@gpshead
Copy link
Member

gpshead commented Dec 11, 2022

New warnings from existing code paths should not normally happen in patch releases. It is unfortunate that this change already shipped in 3.10.9 and 3.11.1. I recommend undoing it in both branches.

Release manager @pablogsal, Is this worth rolling 3.10.10 and 3.11.2 releases for? At least two projects disrupted by the warning change have linked to this issue so far (python-prompt-toolkit and tqdm). There will presumably always be people using 3.10.9 and 3.11.1 so impacted projects may need to carry workarounds forever regardless. Questions to consider:

  • Is that just in those project's tests where DeprecationWarning is enabled?
  • Or does it impact their transitive users?
  • Who else calls the API in this situation getting implicit event loop creation that we don't yet know about?

From #93453:

But then we will have a weird situation, when some code emits a Deprecation warning in 3.10-3.11, but works without warning before 3.10 and after 3.11. Should we undeprecate this case in the future bugfix releases of 3.10 and/or 3.11? It would be not so easy.

The thing to do in this kind of situation is to either leave the existing warning as is in the stable releases, or remove it entirely. (It is fine if we marked something as deprecated in a release and later change our mind and opt not to deprecate it).

What I think happened here appears to be that an attempt to narrow the warning to reflect a new plan was made. This wound up moving the warning to happen from different API calls. The original released warning was removed (fine), but a new location raising the warning was added to a different call path. As seen in the tests: test_get_event_loop() added a new assertion that a DeprecationWarning was raised. That should be a red flag in stable releases. This kind of hiccup might be expected in a first patch 3.11.1 if it were correcting an accident for a warning just added in 3.11, but is never expected in a later lifecycle patch like 3.10.9.

We still need a Deprecation warning in 3.10-3.11 for the case when a new event loop is implicitly created.

Strictly speaking this was a bug that the warning wasn't raised in those releases. The lack of warning about an upcoming behavior change should be considered a reason to delay making the change. ie: restore the implicit event loop creation in 3.12 and remove it in 3.14. I reopened #93453 with such a comment.

@serhiy-storchaka
Copy link
Member

Plan A:

  1. Remove any deprecation warnings in 3.10 and 3.11.
  2. Restore implicit creation of an event loop in 3.12 and add a deprecation warning.
  3. Keep a warning in 3.13.
  4. Remove implicit creation of an event loop in 3.14.

Plan B:

  1. Remove any deprecation warnings in 3.10.
  2. Keep a deprecation warning in 3.11.
  3. Restore implicit creation of an event loop in 3.12 and add a deprecation warning.
  4. Remove implicit creation of an event loop in 3.13.

Plan C:

  1. Do nothing (except maybe changing the documentation more).

@jaraco
Copy link
Member Author

jaraco commented Dec 11, 2022

  • Or does it impact their transitive users?

For prompt toolkit, it does affect transitive users including those of xonsh, who see the warning emitted when the shell starts.

@gvanrossum
Copy link
Member

I think it has to be plan A.

@blink1073
Copy link

FWIW you can add several jupyter packages to affected users. We would also prefer plan A.

@gvanrossum
Copy link
Member

@serhiy-storchaka Is there a PR yet? Should I label this as release blocker for the next 3.10 and 3.11 releases?

@pablogsal pablogsal added release-blocker 3.11 only security fixes 3.10 only security fixes labels Dec 21, 2022
@pablogsal
Copy link
Member

pablogsal commented Dec 21, 2022

I think it has to be plan A.

I concur with this

Should I label this as release blocker for the next 3.10 and 3.11 releases?

I went ahead and add it myself 👍

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Dec 21, 2022
Partially revert changes made in pythonGH-93453.

asyncio.DefaultEventLoopPolicy.get_event_loop() now emits a
DeprecationWarning and creates and sets a new event loop instead of
raising a RuntimeError if there is no current event loop set.
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Dec 21, 2022
gvanrossum added a commit that referenced this issue Jan 10, 2023
…t_loop() (#100412)

Some deprecation warnings will reappear (in a slightly different form) in 3.12.

Co-authored-by: Guido van Rossum <guido@python.org>
@gvanrossum
Copy link
Member

I believe this is now resolved for 3.11 (both code and docs update) but we need a version for 3.10 still. @serhiy-storchaka can you make a backport?

I believe in 3.12 (main) we still need to restore the deprecation warning too. Is that GH-100410?

serhiy-storchaka added a commit that referenced this issue Jan 13, 2023
…H-100410)

Partially revert changes made in GH-93453.

asyncio.DefaultEventLoopPolicy.get_event_loop() now emits a
DeprecationWarning and creates and sets a new event loop instead of
raising a RuntimeError if there is no current event loop set.

Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
@bdarnell
Copy link
Contributor

bdarnell commented Feb 7, 2023

So after deprecation warnings have been added and removed (and moved from one version to another), is this an accurate summary of the long-term plans?

There are three non-deprecated ways to run an event loop:

  1. asyncio.run(). The recommended pattern for most application is an async def main() with asyncio.run(main()).
  2. asyncio.Runner() (new in Python 3.11). This permits starting and stopping the event loop multiple times. This is used for example in unittest.IsolatedAsyncioTestCase.
  3. Explicitly create an event loop, either with asyncio.new_event_loop() or by instantiating a concrete type like asyncio.SelectorEventLoop(), then calling one of its run_* methods.
    1. asyncio provides a thread-local variable which may be used for this event loop with asyncio.set_event_loop and asyncio.get_event_loop

The fourth, deprecated way is to use get_event_loop as in option 3.1 but relying on implicit creation of the loop instead of doing so explicitly.

Historically, asyncio.get_event_loop() did three things:

  1. Return the running event loop if there is one (same as asyncio.get_running_loop())
  2. Return a non-running event loop associated with the current thread, if any
  3. Create an event loop, associate it with the current thread, and return it.

In Python 3.10 the plan was to deprecate the second and third functions of get_event_loop, leaving it as an alias for get_running_loop. But the new plan is to keep the thread-local reference and to only deprecate and remove the automatic creation of the event loop.

Have I got this all right? In particular I want to confirm that get/set_event_loop is once again something that I can rely on for the long term and the changes from 3.10 are not just delayed for a few more releases.

@gvanrossum
Copy link
Member

Yes, this is what we're planning to do. get_event_loop() will no longer create a new event loop (it was always a bit unpredictable whether it would or wouldn't) but it will still let you stash away a loop, associated to a thread, for later use.

There's one thing I'm not 100% sure of, and that is that get_event_loop() returns the running loop, if there is one, even if it differs from the stashed-away loop. This wasn't always the case (IIRC in the dark ages of asyncio there wasn't even an explicit concept of "the running loop" and get_event_loop() always returned the stashed-away loop), but it seems a reasonable compromise. We could possibly start warning if the running loop and the stashed-away loop differ (since it would likely be a user error to get into this situation) but I'm not sure if that's worth it.

@bdarnell
Copy link
Contributor

bdarnell commented Feb 7, 2023

Great, thanks.

My opinion is that if there's a running event loop that's always what you want. If you change get_event_loop so it only returns the stashed-away loop as a counterpart to set_event_loop, many callers will just have to change to try: get_running_loop(); except: get_event_loop(), so I think it's better to keep the current behavior as-is (or to get equivalent behavior in a different way. Tornado's solution for this problem is for the event loop to modify the stashed-away reference when it starts running (and restore the old value when it stops).

I'd be supportive of adding warnings or errors to prevent situations in which the stashed-away event loop differs from the running one. Is it worth it? I think it probably is. One of the reasons these deprecation changes started in Python 3.10 was that it was easy to refer to the stashed-away event loop while setting up your application, and then have those references become invalid when you later call asyncio.run(). The original deprecation plan solved this by getting rid of the stashed-away reference, but now it's back.

I think it's a good idea for asyncio.run() to warn if there's a stashed-away event loop, and possibly in other cases. Any event loop run method could warn if there's a stashed-away event loop that is not self, although asyncio.run() is the main case I'd be concerned about.

@gvanrossum
Copy link
Member

I'd be wary of too many such checks, as this might be performance sensitive. But I like the idea of at least checking in run() (and Runner().run()), initially issuing a warning, eventually raising an error.

@kumaraditya303 What do you think?

@bdarnell
Copy link
Contributor

bdarnell commented Feb 8, 2023

My opinion is that if there's a running event loop that's always what you want. If you change get_event_loop so it only returns the stashed-away loop as a counterpart to set_event_loop, many callers will just have to change to try: get_running_loop(); except: get_event_loop(), so I think it's better to keep the current behavior as-is

After testing the latest 3.12 alpha, I've revised my opinions here. There's no good way today to get the behavior that will be the standard in 3.14. The stashed-away event loop is accessible only via get_event_loop(), which also has the undesirable side effect of logging a deprecation warning and creating a new event loop if there isn't one. (My present use case is that I want to save the stashed-way event loop if there is one, so I can restore it later after temporarily overwriting it. This bit of logic has always wastefully created an unneeded event loop but it didn't come to my attention until the deprecation warning was added).

So while I hate to suggest adding new interfaces in an area where we're actively trying to trim down and simplify things, if there were a function like get_saved_event_loop() that only read from the thread-local reference, I'd use it here (the alternative, I'm afraid, is probably going to be using warnings.catch_warnings in production code paths)

@gvanrossum
Copy link
Member

Hm, it's either that or not call it from the main loop. :-)

A get_saved_event_loop() method on the BaseDefaultEventLoopPolicy class would make sense. I guess we'd have to add it to AbstractEventLoopPolicy as well, maybe with an implementation that just calls self.get_event_loop(), so classes that claim to fully implement AbstractEventLoopPolicy from scratch won't suddenly be out of compliance (though I suspect that uvloop's policy class probably doesn't actually inherit from that, so it might still not work there -- and it can't work in 3.11 or before of course).

I'd rather not add a corresponding top-level function, to minimize API bloat.

@bdarnell
Copy link
Contributor

bdarnell commented Feb 8, 2023

Good news: I think the Tornado code in question is obsolete. Looking back at the history this logic appears to have been an attempt to emulate get_running_loop before asyncio itself had this concept (added in 3.7, FYI). Now that we no longer support those older versions of Python, I can remove this code with no apparent ill effect (at least not anything detected by our CI suite).

I no longer have an immediate need for a get_saved_event_loop() method. It still fills a theoretical hole in the API but unless someone else will speak up for it I doubt it's worth the API bloat at this point.

@CaselIT
Copy link

CaselIT commented Dec 4, 2023

So the 3.12+ best api to get a the stashed loop or a new one is the following?

import asyncio
import sys
import warnings
def get_event_loop() -> asyncio.AbstractEventLoop:
    try:
        return asyncio.get_running_loop()
    except RuntimeError:
        pass
    if sys.version_info < (3,12):
        return asyncio.get_event_loop()
    else:
        with warnings.catch_warnings(action='error'):
            loop = None
            try:
                loop = asyncio.get_event_loop()
            except (DeprecationWarning, RuntimeError):
                pass
            if loop is None:
                loop = asyncio.new_event_loop()
                asyncio.set_event_loop(loop)
            return loop

It may be me, but would have hoped for something simpler

@blink1073
Copy link

That's essentially the approach we had to take in jupyter.

@gvanrossum
Copy link
Member

gvanrossum commented Dec 6, 2023

The problem is that get_event_loop() is trying to be too many things at once -- it gets the running loop, it may create a loop and stash it, or it may return the stashed loop. And heaven help you if there's a running loop and it's different from the stashed loop. Oh, and it will only create a loop in the main thread, and only once, and not if you ever used set_event_loop(). And it will raise if it cannot return a loop, and warn if it decides to create a loop (since that's deprecated).

Rather than the code posted by @CaselIT or the roughly equivalent Jupyter code linked by @blink1073, it's easy to write your own, dropping the "maybe create a loop" functionality. Untested:

class LoopHelper(threading.local):
    loop = None
helper = LoopHelper()
def get_event_loop_or_none():
    try:
        return asyncio.get_running_loop()
    except RuntimeError:
        return helper.loop
def get_event_loop_or_error():
    loop = get_event_loop_or_none()
    if loop is None:
        raise RuntimeError(...)
    return loop
def get_event_loop_or_create():
    loop = get_event_loop_or_none()
    if loop is None:
        loop = helper.loop = asyncio.new_event_loop()
    return loop

(Read "get_event_loop_or_XXX" as "get the running event loop, or the thread-local stored loop if not None, or XXX".)

Manipulate helper.loop to explicitly manage it (maybe write accessors). Use it (or those accessors) directly to skip calling get_running_loop(). Use the latter if you don't care about a stored loop.

@CaselIT
Copy link

CaselIT commented Dec 6, 2023

Thanks for the reply

The problem is that get_event_loop() is trying to be too many things at once

I agree, but maybe some lower level api could make sense? Even just a "get_stashed_loop" would greatly simplify this use case.

def get_event_loop():
  loop = asyncio.get_stashed_loop()  # or raise runtime error if missing
  if loop is None:
    loop = asyncio.new_event_loop()
    asyncio.set_event_loop(loop)
  return loop

In the end I've managed to refactor the code to make use of Runner (but it requires implementing it for python<3.11. Not that adding a new api would be any better in this regard)

@blink1073
Copy link

@gvanrossum what we're trying to guard against is an asyncio loop that was created without our control, but is not currently running.

@gvanrossum
Copy link
Member

@blink1073 If you're serious about proposing a new lower-level API, can you start a discussion on discuss.python.org? I think it's a bit out of scope for this particular issue. (Maybe wait until the new year, many people are relaxing for the coming week or so.)

@blink1073
Copy link

@gvanrossum I've found a better path forward - ensuring that we call set_event_loop prior to initializing any Jupyter app, and storing the event loop for the current thread in a contextvar. jupyter/jupyter_core#387

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.10 only security fixes 3.11 only security fixes release-blocker topic-asyncio type-bug An unexpected behavior, bug, or error
Projects
Development

No branches or pull requests

10 participants