-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-3606 - Document best practice for closing MongoClients and cursors #2465
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
base: master
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1263,10 +1263,14 @@ async def next(self) -> _DocumentType: | |
self._exhaust_checked = True | ||
await self._supports_exhaust() | ||
if self._empty: | ||
if self._cursor_type == CursorType.NON_TAILABLE: | ||
await self.close() | ||
raise StopAsyncIteration | ||
if len(self._data) or await self._refresh(): | ||
return self._data.popleft() | ||
else: | ||
if self._cursor_type == CursorType.NON_TAILABLE: | ||
await self.close() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this close is redundant since _send_message() already calls close after receiving the final batch. Or is there another case where the cursor would be left open? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the use case when a cursor is not automatically closed (outside of a context manager) is when a user doesn't fully iterate through it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah only an non-exhausted cursor is left open. Even then the cursor is closed at a later point by our GC hooks (via the kill cursor thread) but closing explicitly has stronger guarantees. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case, I think a docstring update alone is sufficient. Explicitly calling out that if you don't fully iterate a cursor, you should close it explicitly feels much more actionable than "always use a context manager". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Docs only SGTM. It could explain the close behavior and then say something like "The best practice to ensure the cursor is always closed promptly is to use a context manager..." |
||
raise StopAsyncIteration | ||
|
||
async def _next_batch(self, result: list, total: Optional[int] = None) -> bool: # type: ignore[type-arg] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -202,6 +202,12 @@ def __init__( | |
exception (recognizing that the operation failed) and then continue to | ||
execute. | ||
Best practice is to call :meth:`AsyncMongoClient.close` when the client is no longer needed, | ||
or use the client in a with statement:: | ||
with AsyncMongoClient(url) as client: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be "async with"? |
||
# Use client here. | ||
The `host` parameter can be a full `mongodb URI | ||
<https://dochub.mongodb.org/core/connections>`_, in addition to | ||
a simple hostname. It can also be a list of hostnames but no more | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a bug fix? If so should we create a new bug ticket for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll open a ticket for this. Empty cursors should always be immediately closed.