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

gh-93453: No longer create an event loop in get_event_loop() #98440

Merged
merged 6 commits into from
Dec 6, 2022

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Oct 19, 2022

asyncio.get_event_loop() now always return either running event loop or the result of get_event_loop_policy().get_event_loop() call. The latter should now raise an RuntimeError if no current event loop was set instead of creating and setting a new event loop.

It affects also a number of asyncio functions and constructors which call get_event_loop() implicitly: ensure_future(), shield(), gather(), etc.

DeprecationWarning is no longer emitted if there is no running event loop but the current event loop was set.

asyncio.get_event_loop() now always return either running event loop or
the result of get_event_loop_policy().get_event_loop() call. The latter
should now raise an RuntimeError if no current event loop was set
instead of creating and setting a new event loop.

It affects also a number of asyncio functions and constructors which
call get_event_loop() implicitly: ensure_future(), shield(), gather(),
etc.

DeprecationWarning is no longer emitted if there is no running event loop but
the current event loop was set.
@blink1073
Copy link

👍🏼 to this approach. It would allow us to use the following pattern in jupyter_client to be able to create a ready future in a constructor when there is no event loop running:

def __init__(self):
      try:
            self._ready = asyncio.Future()
      except RuntimeError:
            asyncio.set_event_loop(get_event_loop_policy().get_event_loop())
            self._ready = asyncio.Future()

Similarly, in motor (an asyncio version of pymongo), we currently delay the creation of the event loop used by the async client, and could do so in the constructor as well.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a doc nit -- this looks very thorough and I think we should go with this. However, I would like to double check with @1st1. (I also wish @asvetlov could have a look but I don't want to put pressur on him at all.) So let's not merge this yet until we hear from at least one of them.

Comment on lines 58 to 65
.. deprecated:: 3.10
Deprecation warning is emitted if there is no running event loop.
In future Python releases, this function will be an alias of
:func:`get_running_loop`.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should add a note telling user that in 3.10 and 3.11 (at least up to a certain patch level) this situation would emit a warning.

@gvanrossum
Copy link
Member

@serhiy-storchaka Do you want to undraft this? Then we can do a final review and merge it.

@serhiy-storchaka serhiy-storchaka marked this pull request as ready for review November 30, 2022 04:53
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great -- you can merge now! I won't insist on waiting for Yury.

@serhiy-storchaka
Copy link
Member Author

I do not know the original purpose of the deprecation, so I am not sure that it is the right way.

Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but please add a what's new entry for this.

@serhiy-storchaka
Copy link
Member Author

Ah, yes, it was a reason why this PR was a draft. Thank you for a reminder.

@serhiy-storchaka serhiy-storchaka marked this pull request as draft December 1, 2022 12:58
@gvanrossum
Copy link
Member

I do not know the original purpose of the deprecation, so I am not sure that it is the right way.

Users were getting confused by the meaning of get_event_loop(), which sometimes created a new loop and sometimes didn't. They also misunderstood the meaning of set_event_loop() and would create and set a new loop when a loop already existed. We were so frustrated with this that we decided to change the semantics of get_event_loop() to only return the currently-running loop (i.e., get_running_loop()), but since that was a semantic change we introduced a deprecation first.

Then we heard from users who did understand the mechanism and were using it to solve an issue they couldn't easily solve in any other way, and we changed our mind. The current idea is that get_event_loop() should never create a new loop (it's better to have control over that and use new_event_loop() or better asyncio.run()) but it should still be possible to set a loop in the policy object's thread locals, and get that.

The PR implements that, AFAICT.

@serhiy-storchaka
Copy link
Member Author

Thank you. I know about problems with creating a new loop in get_event_loop(). Initially Andrew was going to deprecate get_event_loop() at all, but I proposed to keep in case if it returns an existing loop, so the existing code written before introduction of get_running_loop() will not need changes. But a corner case, when get_event_loop() returns an existing loop which is not a running loop was not clear to me.

@serhiy-storchaka
Copy link
Member Author

I am planning to merge this PR only after creating a PR for 3.11 to undeprecate what was incorrectly deprecated.

@serhiy-storchaka
Copy link
Member Author

See #99949.

@serhiy-storchaka serhiy-storchaka marked this pull request as ready for review December 2, 2022 11:12
@ambv
Copy link
Contributor

ambv commented Dec 5, 2022

NOTE: this PR assumes the fix backports will be landed in time for 3.10.9 and 3.11.1. This is currently in question (see GH-99949) so please monitor the 3.10 and 3.11 PRs before landing this one.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM. But yeah, let's wait until the 3.11/3.10 PRs have landed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants