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

NoahStapp · 2024-09-05T15:20:15Z

No description provided.

blink1073 · 2024-09-05T17:14:07Z

pymongo/asynchronous/mongo_client.py

@@ -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


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.

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

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

We can remove this comment now.

ShaneHarvey · 2024-09-05T17:35:49Z

pymongo/asynchronous/mongo_client.py

+                    source=self,
+                )
+            if _IS_SYNC and self._opened:
+                self.close()  # type: ignore[unused-coroutine]


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

Ooh good point.

ShaneHarvey · 2024-09-05T17:37:12Z

pymongo/asynchronous/mongo_client.py

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


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

What kind of information should be here? Something like:

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

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

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.

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

ShaneHarvey · 2024-09-05T20:23:27Z

@NoahStapp could you also do some history digging into __del__? I remember there was a reason we avoided adding it to client in the past. Something related to reference cycles.

NoahStapp · 2024-09-05T20:49:06Z

@NoahStapp could you also do some history digging into __del__? I remember there was a reason we avoided adding it to client in the past. Something related to reference cycles.

My understanding is that if you try to do any actual cleanup inside __del__, you risk potentially causing issues with the entire reference cycle your object is part of. All of the past tickets I found on this topic relate to attempting to do cleanup: since this version of __del__ is only raising a warning if the client wasn't explicitly closed before garbage collection, we should avoid any issues.

ShaneHarvey · 2024-09-05T20:57:50Z

Yes that's one issue but there's another unrelated issue. Adding __del__ to a class changes the way CPython garbage collects the object. In particular if it's part of a reference cycle the GC can be significantly delayed because __del__ is defined. I'm going based on memory so this is a bit fuzzy and CPython could work differently now.

ShaneHarvey · 2024-09-05T21:07:24Z

pymongo/asynchronous/mongo_client.py

+        # TODO: Remove in https://jira.mongodb.org/browse/PYTHON-4731
+        try:
+            if not self._closed:
+                if _IS_SYNC:


I'd prefer to remove the await from the error message to reduce the duplication here.

ShaneHarvey · 2024-09-05T21:08:28Z

pymongo/asynchronous/mongo_client.py

+                if _IS_SYNC:
+                    msg = (
+                        f"Unclosed {type(self)}. "
+                        f"Call {type(self)}.close() to safely shut down your client and free up resources."


The error message can/should also include the traceback of where the client was opened via topology_settings._stack.

ShaneHarvey · 2024-09-05T21:09:09Z

pymongo/asynchronous/mongo_client.py

@@ -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


We can remove this comment now.

NoahStapp · 2024-09-06T14:02:09Z

Yes that's one issue but there's another unrelated issue. Adding __del__ to a class changes the way CPython garbage collects the object. In particular if it's part of a reference cycle the GC can be significantly delayed because __del__ is defined. I'm going based on memory so this is a bit fuzzy and CPython could work differently now.

I can't find an example of __del__ delaying GC, but I did find this: the garbage collector docs suggest that starting in 3.4, defining __del__ shouldn't affect the GC behavior for that object, even if it is part of a reference cycle.

ShaneHarvey · 2024-09-06T18:36:51Z

Thanks for looking into it. Could you?:

add changelog entry
merge master to get the tests green
provide an example of the new warning behavior

NoahStapp · 2024-09-06T18:51:49Z

Here's the current warning:

sys:1: ResourceWarning: Unclosed MongoClient opened at:
  File "/Users/nstapp/Github/mongo-python-driver/scratch.py", line 8, in <module>
    client = MongoClient()
  File "/Users/nstapp/Github/mongo-python-driver/pymongo/synchronous/mongo_client.py", line 847, in __init__
    self._topology_settings = TopologySettings(
  File "/Users/nstapp/Github/mongo-python-driver/pymongo/synchronous/settings.py", line 85, in __init__
    self._stack = "".join(traceback.format_stack())
Call MongoClient.close() to safely shut down your client and free up resources.

ShaneHarvey · 2024-09-06T18:57:01Z

What about the "Call {type(self).name}.close() to safely shut down your client and free up resources." part?

ShaneHarvey · 2024-09-06T18:58:20Z

Also I wonder if we can remove the last two frames from the trace to make it cleaner.

NoahStapp · 2024-09-06T19:00:24Z

What about the "Call {type(self).name}.close() to safely shut down your client and free up resources." part?

Fixed.

NoahStapp · 2024-09-06T19:00:58Z

Also I wonder if we can remove the last two frames from the trace to make it cleaner.

Return just the first frame? That should always be the line where the client was constructed, right?

ShaneHarvey · 2024-09-06T19:02:49Z

I mean

self._stack = "".join(traceback.format_stack()[:-2])

ShaneHarvey · 2024-09-06T19:41:12Z

doc/changelog.rst

@@ -42,6 +42,8 @@ PyMongo 4.9 brings a number of improvements including:
 - Fixed a bug where PyMongo would raise ``InvalidBSON: date value out of range``
  when using :attr:`~bson.codec_options.DatetimeConversion.DATETIME_CLAMP` or
  :attr:`~bson.codec_options.DatetimeConversion.DATETIME_AUTO` with a non-UTC timezone.
+- Added a warning to unclosed MongoClient instances
+  telling users to explicitly close clients when finished with them to avoid leaking resources .


resources . -> resources.

Let's include an example of the warning here too.

ShaneHarvey · 2024-09-06T20:30:47Z

doc/changelog.rst

@@ -42,6 +42,16 @@ PyMongo 4.9 brings a number of improvements including:
 - Fixed a bug where PyMongo would raise ``InvalidBSON: date value out of range``
  when using :attr:`~bson.codec_options.DatetimeConversion.DATETIME_CLAMP` or
  :attr:`~bson.codec_options.DatetimeConversion.DATETIME_AUTO` with a non-UTC timezone.
+- Added a warning to unclosed MongoClient instances
+  telling users to explicitly close clients when finished with them to avoid leaking resources.
+  Here's an example of what the warning looks like:


Nit: Here's an example of what the warning looks like: -> For example:

blink1073

LGTM!

Addressed

PYTHON-3193 - Add ResourceWarning for unclosed MongoClients in __del__

d7d8ce8

mongodb-drivers-pr-bot bot requested a review from caseyclements September 5, 2024 15:24

Fix typecheck

f5fbeef

NoahStapp requested a review from blink1073 September 5, 2024 15:44

blink1073 removed the request for review from caseyclements September 5, 2024 17:10

blink1073 reviewed Sep 5, 2024

View reviewed changes

ShaneHarvey requested changes Sep 5, 2024

View reviewed changes

Address review

2bac24c

ShaneHarvey previously requested changes Sep 5, 2024

View reviewed changes

Add traceback

c9f99a0

NoahStapp added 2 commits September 6, 2024 14:45

Merge branch 'master' into PYTHON-3193

928b164

Update changelog

3c3e305

fix traceback

f8ef260

NoahStapp requested a review from ShaneHarvey September 6, 2024 19:20

ShaneHarvey reviewed Sep 6, 2024

View reviewed changes

Add warning example to changelog

bf6868a

NoahStapp requested a review from ShaneHarvey September 6, 2024 20:26

ShaneHarvey reviewed Sep 6, 2024

View reviewed changes

Address review

9f3bd1d

Merge branch 'master' into PYTHON-3193

7a6ba04

NoahStapp requested a review from ShaneHarvey September 9, 2024 12:14

blink1073 approved these changes Sep 9, 2024

View reviewed changes

NoahStapp merged commit 2cca2d9 into mongodb:master Sep 9, 2024
34 checks passed

NoahStapp deleted the PYTHON-3193 branch September 9, 2024 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

NoahStapp commented Sep 5, 2024

blink1073 Sep 5, 2024 •

edited

Loading

ShaneHarvey Sep 5, 2024

blink1073 Sep 5, 2024

ShaneHarvey Sep 5, 2024

ShaneHarvey Sep 5, 2024

NoahStapp Sep 5, 2024

ShaneHarvey Sep 5, 2024 •

edited

Loading

NoahStapp Sep 5, 2024 •

edited

Loading

ShaneHarvey Sep 5, 2024

NoahStapp Sep 5, 2024 •

edited

Loading

ShaneHarvey Sep 5, 2024

ShaneHarvey commented Sep 5, 2024

NoahStapp commented Sep 5, 2024

ShaneHarvey commented Sep 5, 2024

ShaneHarvey Sep 5, 2024

ShaneHarvey Sep 5, 2024

ShaneHarvey Sep 5, 2024

NoahStapp commented Sep 6, 2024

ShaneHarvey commented Sep 6, 2024

NoahStapp commented Sep 6, 2024 •

edited

Loading

ShaneHarvey commented Sep 6, 2024

ShaneHarvey commented Sep 6, 2024

NoahStapp commented Sep 6, 2024

NoahStapp commented Sep 6, 2024

ShaneHarvey commented Sep 6, 2024

ShaneHarvey Sep 6, 2024

ShaneHarvey Sep 6, 2024

blink1073 left a comment

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

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

Conversation

NoahStapp commented Sep 5, 2024

blink1073 Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

NoahStapp Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoahStapp Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey commented Sep 5, 2024

NoahStapp commented Sep 5, 2024

ShaneHarvey commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoahStapp commented Sep 6, 2024

ShaneHarvey commented Sep 6, 2024

NoahStapp commented Sep 6, 2024 • edited Loading

ShaneHarvey commented Sep 6, 2024

ShaneHarvey commented Sep 6, 2024

NoahStapp commented Sep 6, 2024

NoahStapp commented Sep 6, 2024

ShaneHarvey commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blink1073 left a comment

Choose a reason for hiding this comment

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

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

blink1073 Sep 5, 2024 •

edited

Loading

ShaneHarvey Sep 5, 2024 •

edited

Loading

NoahStapp Sep 5, 2024 •

edited

Loading

NoahStapp Sep 5, 2024 •

edited

Loading

NoahStapp commented Sep 6, 2024 •

edited

Loading