Opened 4 weeks ago

Last modified 8 days ago

#36701 assigned Cleanup/optimization

ModelState objects create reference cycles that require a gc pass to free

Reported by: Patryk Zawadzki Owned by: 고명주
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: memory gc modelstate fields_cache
Cc: Patryk Zawadzki Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Disclaimer: it's impossible for pure Python code to truly leak memory (in the sense that valgrind would detect), however it's quite easy to create structures that effectively occupy memory for a long time because they require the deepest (generation 2) garbage collection cycle to collect and that happens very rarely. In addition to that, the more such structures aggregate, the more expensive the garbage collection cycle becomes, because it effectively stops the entire interpreter to do its job and it can take seconds. On top of that, it's entirely possible for a container to run out of memory before the garbage collection happens and we (Saleor Commerce) see containers being terminated by the kernel OOM killer due to high memory pressure where most of that memory is locked by garbage.

Each model instance includes a ModelState object that in turn contains references to other model instances. It's possible for those to be cyclic. In fact, the simplest cyclic case is a OneToOneField that links the states of both objects to the opposite side as soon as the relationship is traversed.

  1. When foo.bar is evaluated, the ForwardManyToOneDescriptor fetches the related Bar object and sets foo._state.fields_cache["bar"] to the retrieved instance (through a call to Field.set_cached_value).
  2. If the relation is not multiple (so a one-to-one), it also sets bar._state.fields_cache["foo"] to the foo object on the Bar instance.

While OneToOneField is the easiest way to create such a cycle, it's also possible to create them through manual assignment (foo.bar = bar, bar.baz = baz, baz.foo = foo), although one might argue that a manually created cycle is the fault and therefore concern of the user.

The easiest way to solve this is to break the reference cycle by implementing a finalizer (__del__ method) on either the base Model class to remove the state (del self._state), or the ModelState class to remove the field cache (self.fields_cache.clear()).

Attachments (1)

image-20251031-152647.png (56.0 KB ) - added by Patryk Zawadzki 4 weeks ago.

Download all attachments as: .zip

Change History (7)

by Patryk Zawadzki, 4 weeks ago

Attachment: image-20251031-152647.png added

comment:1 by Jacob Walls, 4 weeks ago

Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization
Version: 5.2dev

Hi Patryk, thanks for the careful report. Incremental garbage collection is new in Python 3.14 and might make most of your headaches here go away:

This means that maximum pause times are reduced by an order of magnitude or more for larger heaps.
There are now only two generations: young and old.

The behavior difference between foreign keys and one-to-one fields here is surprising, though. Tentatively accepting so we can look at a patch. Thanks.

comment:2 by Varun Kasyap Pentamaraju, 4 weeks ago

Owner: set to Varun Kasyap Pentamaraju
Status: newassigned

comment:3 by Varun Kasyap Pentamaraju, 4 weeks ago

Hi,

I tried adding the __del__ method to ModelState as suggested.

I also wrote a gc test to check for the OneToOneField reference cycle. The test is still failing, and the objects are not being collected.

I am worried that adding __del__ to an object inside a reference cycle might be preventing the garbage collector from cleaning it up. This __del__ approach might not be the right fix here.

comment:4 by Patryk Zawadzki, 3 weeks ago

Hey, we have a working internal workaround that adds __del__ to model classes and just does del self._state and it seems to work great.

comment:5 by Varun Kasyap Pentamaraju, 3 weeks ago

Owner: Varun Kasyap Pentamaraju removed
Status: assignednew

comment:6 by 고명주, 8 days ago

Owner: set to 고명주
Status: newassigned
Note: See TracTickets for help on using tickets.
Back to Top