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-938 Docs for watch() incorrectly call ChangeStream.close() #163

Merged
merged 6 commits into from Apr 28, 2022

Conversation

blink1073
Copy link
Member

@blink1073 blink1073 commented Apr 25, 2022

No description provided.

@blink1073 blink1073 changed the title MOTOR-938Docs for watch() incorrectly call ChangeStream.close() MOTOR-938 Docs for watch() incorrectly call ChangeStream.close() Apr 25, 2022
motor/core.py Outdated
if change_stream is not None:
change_stream.close()
asyncio.run(change_stream.close())
Copy link
Member

@ShaneHarvey ShaneHarvey Apr 25, 2022

Choose a reason for hiding this comment

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

Can you confirm that the close() is not needed? I believe the async with collection.watch() as change_stream line will handle closing the cursor when KeyboardInterrupt is raised to asycnio.run().

Copy link
Member Author

@blink1073 blink1073 Apr 26, 2022

Choose a reason for hiding this comment

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

I did some local testing, ensuring that the change_stream was actually closed. asyncio.run properly runs __aexit__(), closing the stream. For the tornado loop, I had to manually run the close operation.

if change_stream is not None:
change_stream.close()
loop.add_callback(change_stream.close)
Copy link
Member

@ShaneHarvey ShaneHarvey Apr 25, 2022

Choose a reason for hiding this comment

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

Does this actually work? I would expect close to never run since the loop is not running at this point. What if we use loop.run_sync similar to asyncio.run?:

    loop = tornado.ioloop.IOLoop.current()
    # Start watching collection for changes.
    try:
        loop.run_sync(lambda: watch(collection))
    except KeyboardInterrupt:
        pass

@blink1073 blink1073 requested a review from ShaneHarvey Apr 26, 2022
finally:
if change_stream is not None:
change_stream.close()
pass
Copy link
Contributor

@juliusgeo juliusgeo Apr 26, 2022

Choose a reason for hiding this comment

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

Does this pass need to be here?

Copy link
Member Author

@blink1073 blink1073 Apr 26, 2022

Choose a reason for hiding this comment

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

Yes, that or a comment, or it would be a syntax error.

@blink1073 blink1073 requested a review from juliusgeo Apr 27, 2022
Copy link
Contributor

@juliusgeo juliusgeo left a comment

LGTM!

@blink1073 blink1073 merged commit 19537bb into mongodb:master Apr 28, 2022
24 checks passed
@blink1073 blink1073 deleted the MOTOR-938 branch Apr 28, 2022
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