Fixed #36701 -- Fixed memory leak in ModelState reference cycles #20380

Samriddha9619 · 2025-12-08T16:46:26Z

Trac ticket number

Branch description

This PR addresses ticket #36701 regarding memory leaks caused by reference cycles in ModelState

I implemented a __del__ method on ModelState to explicitly clear self.fields_cache.

I added a regression test (tests/model_state/test_memory_leak.py) that:

Creates a OneToOneField cycle.
Deletes strong references.
Forces a gc.collect().
Asserts that the objects are successfully reclaimed (using weakref).

I also verified this locally with a reproduction script generating 50,000 cyclic objects. Without the fix, memory usage balloons. With the fix, gc.collect() successfully reclaims the objects.

Checklist

This PR targets the main branch.
The commit message is written in past tense, mentions the ticket number, and ends with a period.
I have checked the "Has patch" ticket flag in the Trac system.
I have added or updated relevant tests.
I have added or updated relevant docs, including release notes if applicable.
I have attached screenshots in both light and dark modes for any UI changes.

tests/model_state/test_memory_leak.py

jacobtylerwalls

Thanks 👍

jacobtylerwalls · 2025-12-08T21:15:08Z

django/db/models/base.py

+        except AttributeError:
+            pass


Why do we need the except?

Thanks for explaining -- in that case these lines are reachable, so we need a test to cover them.

jacobtylerwalls · 2025-12-08T21:17:48Z

tests/model_regress/test_state.py

+        self.addCleanup(gc.set_debug, gc.get_debug())
+        gc.set_debug(gc.DEBUG_SAVEALL)
+
+        gc.collect()


Please use the garbage_collect() helper from django.test.utils.

jacobtylerwalls · 2025-12-08T21:20:27Z

tests/model_regress/test_state.py

+        gc.set_debug(gc.DEBUG_SAVEALL)
+
+        gc.collect()
+        del gc.garbage[:]


Why do we need this?

jacobtylerwalls · 2025-12-08T21:21:01Z

tests/model_regress/test_state.py

+        """
+        Ensure OneToOneField intermediate state does not create reference
+        cycles during model initialization.
+        """


Suggested change

"""

Ensure OneToOneField intermediate state does not create reference

cycles during model initialization.

"""

Thanks for the method name change. Now we can lose the docstring (we avoid "Ensure ..." preambles anyway.)

jacobtylerwalls · 2025-12-08T21:28:08Z

tests/model_regress/test_state.py

+
+        leaked = [obj for obj in gc.garbage if id(obj) == p_id]
+
+        self.assertEqual(leaked, [])


After this test I get this warning:

Exception ignored in GC shutdown: ResourceWarning: gc: 7995 uncollectable objects at shutdown; use gc.set_debug(gc.DEBUG_UNCOLLECTABLE) to list them

Again, thanks very much for explaining. Now we need to register this as a cleanup earlier (right after garbage_collect, with self.addCleanup) because if the assertion fails for some reason I get the warning again.

Samriddha9619 · 2025-12-09T10:53:23Z

@jacobtylerwalls I tried to write tests using garbage_collect() from django.tests.utils but I was not getting desired results so I switched to a direct test (test_model_state_del_clears_cache). This new test proves that ModelState.__del__ correctly removes the _fields_cache, which breaks the reference cycle.
(If the test is not satisfactory I can try rewriting it again with garbage_collect() as you suggested )

Also removed the try / except block and verified the tests locally

django/db/models/base.py

tests/model_regress/test_state.py

Samriddha9619 · 2025-12-10T20:52:00Z

@jacobtylerwalls
For the try/except block in django/db/models/base.py
I looked into this because you asked when _fields_cache would be missing.
It turns out _fields_cache is lazy-loaded. It doesn't actually exist on a model instance until you access a related field (like a ForeignKey).
I tested this locally if I create a simple Model instance without touching any relationships, _fields_cache isn't there. If I remove this try/except block , __del__ crashes with an AttributeError for those cases. I kept it to handle that default state safely

For the del gc.garbage line in tests/model_regress/test_state.py Since I'm using gc.DEBUG_SAVEALL to catch the cycle, the garbage collector intentionally keeps the leaked objects in gc.garbage instead of deleting them.
If I don't clear that list manually here at the end, the Django test runner detects those 'saved' objects during shutdown and throws a ResourceWarning about uncollectable objects. This line just ensures the test cleans up after itself so the suite stays clean.

github-actions · 2025-12-10T20:52:16Z

📊 Coverage Report for Changed Files

-------------
Diff Coverage
Diff: origin/main...HEAD, staged and unstaged changes
-------------
django/db/models/base.py (60.0%): Missing lines 500-501
-------------
Total:   5 lines
Missing: 2 lines
Coverage: 60%
-------------

Note: Missing lines are warnings only. Some lines may not be covered by SQLite tests as they are database-specific.

For more information about code coverage on pull requests, see the contributing documentation.

jacobtylerwalls

Thanks for the updates 👍

jacobtylerwalls · 2025-12-10T21:24:50Z

django/db/models/base.py

+        except AttributeError:
+            pass


Thanks for explaining -- in that case these lines are reachable, so we need a test to cover them.

jacobtylerwalls · 2025-12-10T21:27:57Z

tests/model_regress/test_state.py

+
+        leaked = [obj for obj in gc.garbage if id(obj) == p_id]
+
+        self.assertEqual(leaked, [])


Again, thanks very much for explaining. Now we need to register this as a cleanup earlier (right after garbage_collect, with self.addCleanup) because if the assertion fails for some reason I get the warning again.

jacobtylerwalls reviewed Dec 8, 2025

View reviewed changes

tests/model_state/test_memory_leak.py Outdated Show resolved Hide resolved

tests/model_state/test_memory_leak.py Outdated Show resolved Hide resolved

tests/model_state/test_memory_leak.py Outdated Show resolved Hide resolved

Samriddha9619 force-pushed the ticket-36701-modelstate-leak branch 2 times, most recently from cbaf05e to 4c184a7 Compare December 8, 2025 18:59

Samriddha9619 requested a review from jacobtylerwalls December 8, 2025 19:10

0saurabh0 mentioned this pull request Dec 8, 2025

Refs #36620 -- Fixed PR number extraction in coverage_comment workflow. #20375

Merged

jacobtylerwalls reviewed Dec 8, 2025

View reviewed changes

Samriddha9619 force-pushed the ticket-36701-modelstate-leak branch from 4c184a7 to 126f444 Compare December 9, 2025 10:36

jacobtylerwalls reviewed Dec 9, 2025

View reviewed changes

django/db/models/base.py Outdated Show resolved Hide resolved

tests/model_regress/test_state.py Outdated Show resolved Hide resolved

Fixed #36701 -- Fixed memory leak in ModelState reference cycles.

78c2504

Samriddha9619 force-pushed the ticket-36701-modelstate-leak branch from 126f444 to 78c2504 Compare December 10, 2025 20:40

Samriddha9619 requested a review from jacobtylerwalls December 10, 2025 20:55

jacobtylerwalls reviewed Dec 10, 2025

View reviewed changes


		leaked = [obj for obj in gc.garbage if id(obj) == p_id]

		self.assertEqual(leaked, [])

Uh oh!

Fixed #36701 -- Fixed memory leak in ModelState reference cycles #20380

Are you sure you want to change the base?

Fixed #36701 -- Fixed memory leak in ModelState reference cycles #20380

Conversation

Samriddha9619 commented Dec 8, 2025

Trac ticket number

Branch description

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jacobtylerwalls left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Samriddha9619 commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Samriddha9619 commented Dec 10, 2025

Uh oh!

github-actions bot commented Dec 10, 2025

📊 Coverage Report for Changed Files

Uh oh!

jacobtylerwalls left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Samriddha9619 commented Dec 9, 2025 •

edited

Loading