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

MOTOR-979 Use asyncio.get_event_loop so that DeprecationWarnings are correctly issued #170

Merged
merged 1 commit into from Jun 16, 2022

Conversation

graingert
Copy link
Contributor

…issued

It looks like a workaround I wrote in Tornado was incorrectly incorporated into AsyncioMotorClient here https://github.com/mongodb/motor/pull/155/files#r851429040

if a user has opted into DeprecationWarnings as errors asyncio.get_event_loop_policy().get_event_loop() incorrectly suppresses the deprecation warning:

Python 3.10.4 (main, Apr  8 2022, 17:35:13) [GCC 9.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import motor.motor_asyncio
>>> client = motor.motor_asyncio.AsyncIOMotorClient()
>>> client.test.pages.drop()  # this should raise a DeprecationWarning because it was called without a running loop
<Future pending cb=[_chain_future.<locals>._call_check_cancel() at /usr/lib/python3.10/asyncio/futures.py:384]>
>>> 

@blink1073 blink1073 changed the title use asyncio.get_event_loop so that DeprecationWarnings are correctly … MOTOR-902 Use asyncio.get_event_loop so that DeprecationWarnings are correctly issued Jun 15, 2022
@ShaneHarvey
Copy link
Member

client.test.pages.drop() # this should raise a DeprecationWarning because it was called without a running loop

I think there's a misunderstanding here. We don't want this line to raise a DeprecationWarning or fail with an error because a loop wasn't created. We intentionally decided that we what this code to continue working like it does in Python <3.9 where motor will implicitly create and start a loop on the current thread. With that in mind, we can't merge this PR. Is there an alternative way you'd suggest accomplishing our goal?

@graingert
Copy link
Contributor Author

This will only raise a DeprecationWarning if the caller has opted into this behavior

@ShaneHarvey
Copy link
Member

We don't want this line to raise a DeprecationWarning even if the user opts into warnings.

@blink1073
Copy link
Member

I think @graingert's point is that we're masking an issue that will cause breakage in Python 3.12 (assuming they stick to their schedule of aliasing get_event_loop() to get_running_loop()).

@ShaneHarvey
Copy link
Member

In this case I think that CPython is incorrectly not raising DeprecationWarning from asyncio.get_event_loop_policy().get_event_loop(). We should open a python bug for that.

Perhaps we should revisit this and just follow asyncio's lead. We'd need to re-evaluate the problem as a whole though. For example, if we create a client before there's a loop (or a running loop) should we auto create a new loop? Or should we pin to the current loop when the client is first used?

@graingert
Copy link
Contributor Author

graingert commented Jun 15, 2022

For example, if we create a client before there's a loop (or a running loop)

This works correctly already, as AsyncIOMotorClient now mostly follows the asyncio.mixins._LoopBoundMixin pattern:

Python 3.10.4 (main, Apr  8 2022, 17:35:13) [GCC 9.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import motor.motor_asyncio
>>> client = motor.motor_asyncio.AsyncIOMotorClient()  # before #155 this raised a DeprecationWarning and bound the wrong loop
>>> client.test.pages.drop()  # this should raise a DeprecationWarning because it was called without a running loop
<Future pending cb=[_chain_future.<locals>._call_check_cancel() at /usr/lib/python3.10/asyncio/futures.py:384]>
>>> 

@ShaneHarvey ShaneHarvey changed the title MOTOR-902 Use asyncio.get_event_loop so that DeprecationWarnings are correctly issued MOTOR-979 Use asyncio.get_event_loop so that DeprecationWarnings are correctly issued Jun 15, 2022
@ShaneHarvey
Copy link
Member

Okay I believe I'm on the same page now. I opened https://jira.mongodb.org/browse/MOTOR-979 to track.

@mongodb mongodb deleted a comment from graingert Jun 15, 2022
Copy link
Member

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

LGTM!

@blink1073
Copy link
Member

I opened https://jira.mongodb.org/browse/MOTOR-980 to track the docs failure. Thanks @graingert!

@blink1073 blink1073 merged commit a77db8f into mongodb:master Jun 16, 2022
@graingert graingert deleted the patch-1 branch June 16, 2022 06:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants