Skip to content

PYTHON-3193 - Add ResourceWarning for unclosed MongoClients in __del__ #1833

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Sep 9, 2024
Merged

PYTHON-3193 - Add ResourceWarning for unclosed MongoClients in __del__ #1833

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d7d8ce8
PYTHON-3193 - Add ResourceWarning for unclosed MongoClients in __del__
NoahStapp Sep 5, 2024
f5fbeef
Fix typecheck
NoahStapp Sep 5, 2024
2bac24c
Address review
NoahStapp Sep 5, 2024
c9f99a0
Add traceback
NoahStapp Sep 6, 2024
928b164
Merge branch 'master' into PYTHON-3193
NoahStapp Sep 6, 2024
3c3e305
Update changelog
NoahStapp Sep 6, 2024
f8ef260
fix traceback
NoahStapp Sep 6, 2024
bf6868a
Add warning example to changelog
NoahStapp Sep 6, 2024
9f3bd1d
Address review
NoahStapp Sep 9, 2024
7a6ba04
Merge branch 'master' into PYTHON-3193
NoahStapp Sep 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions pymongo/asynchronous/mongo_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@

import contextlib
import os
import warnings
import weakref
from collections import defaultdict
from typing import (
Expand Down Expand Up @@ -864,6 +865,7 @@ def __init__(
)

self._opened = False
self._closed = False
self._init_background()

if _IS_SYNC and connect:
Expand Down Expand Up @@ -1173,6 +1175,22 @@ def __getitem__(self, name: str) -> database.AsyncDatabase[_DocumentType]:
"""
return database.AsyncDatabase(self, name)

def __del__(self) -> None:
"""Check that this AsyncMongoClient has been closed and issue a warning if not."""
# TODO: Remove in https://jira.mongodb.org/browse/PYTHON-4731
Copy link
Member

@blink1073 blink1073 Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't realize we removed MongoClient.__del__. I think we should preserve the 4.8 behavior and change connect=False and remove the __del__ behavior together in 5.0.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't we want to keep the __del__ warning forever?

Copy link
Member

Choose a reason for hiding this comment

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

Oh wait, we never had a __del__. Yes, I agree.

Copy link
Member

Choose a reason for hiding this comment

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

We can remove this comment now.

try:
if not self._closed:
warnings.warn(
f"Unclosed {self}",
Copy link
Member

@ShaneHarvey ShaneHarvey Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We should add more information here so users know why/when this started.

Copy link
Contributor Author

@NoahStapp NoahStapp Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What kind of information should be here? Something like:

Unclosed MongoClient. Call MongoClient.close() to safely shut down your client and free up resources.?

Copy link
Member

Choose a reason for hiding this comment

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

We should point users to the docs for https://jira.mongodb.org/browse/PYTHON-3606 (still TODO)

Copy link
Contributor Author

@NoahStapp NoahStapp Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do we want to pull that ticket up to 4.9, then? We should do them both in the same release if we want them to be linked.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. We need good docs or else we're going to be inundated with questions.

ResourceWarning,
stacklevel=2,
source=self,
)
if _IS_SYNC and self._opened:
self.close() # type: ignore[unused-coroutine]
Copy link
Member

Choose a reason for hiding this comment

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

We can't call close() even in the sync api because it might deadlock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh good point.

except AttributeError:
pass

def _close_cursor_soon(
self,
cursor_id: int,
Expand Down Expand Up @@ -1540,6 +1558,7 @@ async def close(self) -> None:
if self._encrypter:
# TODO: PYTHON-1921 Encrypted MongoClients cannot be re-opened.
await self._encrypter.close()
self._closed = True

if not _IS_SYNC:
# Add support for contextlib.aclosing.
Expand Down
19 changes: 19 additions & 0 deletions pymongo/synchronous/mongo_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@

import contextlib
import os
import warnings
import weakref
from collections import defaultdict
from typing import (
Expand Down Expand Up @@ -863,6 +864,7 @@ def __init__(
)

self._opened = False
self._closed = False
self._init_background()

if _IS_SYNC and connect:
Expand Down Expand Up @@ -1172,6 +1174,22 @@ def __getitem__(self, name: str) -> database.Database[_DocumentType]:
"""
return database.Database(self, name)

def __del__(self) -> None:
"""Check that this MongoClient has been closed and issue a warning if not."""
# TODO: Remove in https://jira.mongodb.org/browse/PYTHON-4731
try:
if not self._closed:
warnings.warn(
f"Unclosed {self}",
ResourceWarning,
stacklevel=2,
source=self,
)
if _IS_SYNC and self._opened:
self.close() # type: ignore[unused-coroutine]
except AttributeError:
pass

def _close_cursor_soon(
self,
cursor_id: int,
Expand Down Expand Up @@ -1535,6 +1553,7 @@ def close(self) -> None:
if self._encrypter:
# TODO: PYTHON-1921 Encrypted MongoClients cannot be re-opened.
self._encrypter.close()
self._closed = True

if not _IS_SYNC:
# Add support for contextlib.closing.
Expand Down
3 changes: 3 additions & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,9 @@ filterwarnings = [
"module:please use dns.resolver.Resolver.resolve:DeprecationWarning",
# https://github.com/dateutil/dateutil/issues/1314
"module:datetime.datetime.utc:DeprecationWarning:dateutil",
# TODO: Remove both of these in https://jira.mongodb.org/browse/PYTHON-4731
"ignore:Unclosed AsyncMongoClient*",
"ignore:Unclosed MongoClient*",
]
markers = [
"auth_aws: tests that rely on pymongo-auth-aws",
Expand Down
Loading