django.contrib.messages Storage creates circular references with Request objects

[Raphael Gaschignard]

To be honest I don't know how much people care about this kind of issue, but I am having some (very silly) operational issues downstream of Requests getting garbage collected at awkward times.

Request objects hold onto message storages via Request._messages. The storage classes hold references back to Request. This creates a reference cycle preventing Requests from being collected as fast as they could be (and can create some awkwardness in CPython, with del calls happening on associated objects in seemingly unrelated spots)

The "simple" fix here would be to hold onto request in the storage through a weakref, and add a property to get the request object on the storage class.

[Raphael Gaschignard]

An example of the sort of cycle that appears (objgraph output)

[Manish Tiwari]

Does someone working on this issue, or it's good to go for starting out for this??

[Augusto Pontes]

Kinda complex issue, but thanks to send the image, it clarified a lot, and definitely this circular reference can cause not only garbage collector issues, but in my point of view: performance, i will analyse this module on django core, and see if i can give any suggestions, also, we need to see if writing any changes on this module, can affect not only other modules, but the unit tests

[Jacob Walls]

Thanks, I reproduced by riffing on the test from #34865:

(quick and dirty, not suggesting to edit this test case)

tests/messages_tests/base.py

@@ -330 +330 @@
         self.assertEqual(len(storage), 6)
 
     def test_high_level(self):
+        import gc
+    
+        # Schedule the restore of the garbage collection settings.
+        self.addCleanup(gc.set_debug, 0)
+        self.addCleanup(gc.enable)
+
+        # Disable automatic garbage collection to control when it's triggered,
+        # then run a full collection cycle to ensure `gc.garbage` is empty.
+        gc.disable()
+        gc.collect()
+
+        # The garbage list isn't automatically populated to avoid CPU overhead,
+        # so debugging needs to be enabled to track all unreachable items and
+        # have them stored in `gc.garbage`.
+        gc.set_debug(gc.DEBUG_SAVEALL)
+
         request = self.get_request()
         storage = self.storage_class(request)
         request._messages = storage
@@ -340 +356 @@
         add_level_messages(storage)
         self.assertEqual(len(storage), 2)
 
+        # Dereference.
+        request = None
+        storage = None
+
+        # Enforce garbage collection to populate `gc.garbage` for inspection.
+        gc.collect()
+        self.assertEqual(gc.garbage, [])
+
+
     @override_settings(MESSAGE_LEVEL=29)
     def test_settings_level(self):
         request = self.get_request()

AssertionError: Lists differ: [<HttpRequest>, <QueryDict: {}>, <QueryDic[237 chars], []] != []

First list contains 11 additional elements.
First extra element 0:
<HttpRequest>

+ []
- [<HttpRequest>,
-  <QueryDict: {}>,
-  <QueryDict: {}>,
-  {},
-  {},
-  <MultiValueDict: {}>,
-  <SessionStorage: request=<HttpRequest>>,
-  [Message(level=30, message='A warning'),
-   Message(level=40, message='An error')],
-  Message(level=30, message='A warning'),
-  Message(level=40, message='An error'),
-  []]

The "simple" fix here would be to hold onto request in the storage through a weakref, and add a property to get the request object on the storage class.

I tried that:

django/contrib/messages/storage/base.py

@@ +1 @@
+import weakref
 from django.conf import settings
 from django.contrib.messages import constants, utils
 from django.utils.functional import SimpleLazyObject
@@ -55 +56 @@
     """
 
     def __init__(self, request, *args, **kwargs):
-        self.request = request
+        self._request = weakref.proxy(request)
         self._queued_messages = []
         self.used = False
         self.added_new = False
         super().__init__(*args, **kwargs)
 
+    @property
+    def request(self):
+        return self._request
+
     def __len__(self):
         return len(self._loaded_messages) + len(self._queued_messages)

But I just got:

ERROR: test_add_lazy_translation (messages_tests.test_session.SessionTests.test_add_lazy_translation)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jwalls/django/tests/messages_tests/base.py", line 101, in test_add_lazy_translation
    storage.update(response)
    ~~~~~~~~~~~~~~^^^^^^^^^^
  File "/Users/jwalls/django/django/contrib/messages/storage/base.py", line 145, in update
    return self._store(messages, response)
           ~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^
  File "/Users/jwalls/django/django/contrib/messages/storage/session.py", line 40, in _store
    self.request.session[self.session_key] = self.serialize_messages(messages)
    ^^^^^^^^^^^^^^^^^^^^
ReferenceError: weakly-referenced object no longer exists

----------------------------------------------------------------------
Ran 4 tests in 0.008s

[Jacob Walls]

Tentatively accepting on the premise that there is a non-awkward solution for this given the way this app uses middleware.

[Augusto Pontes]

Replying to Jacob Walls:

Tentatively accepting on the premise that there is a non-awkward solution for this given the way this app uses middleware.

But its still possible to fix it?

[Jacob Walls]

Sure, sharing any additional findings will be a help.

[Augusto Pontes]

I created a thread on the django discord server, here is the link: ​https://discord.gg/4VtuEf2J, the thread has the same name of the ticket here

[Augusto Pontes]

​https://github.com/django/django/pull/19992