[Django] #36662: django.contrib.messages Storage creates circular references with Request objects

[Django]

#36662: django.contrib.messages Storage creates circular references with Request
objects
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Type:
| Cleanup/optimization
Status: new | Component:
| contrib.messages
Version: dev | Severity: Normal
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36662>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36662: django.contrib.messages Storage creates circular references with Request
objects
-------------------------------------+-------------------------------------

Reporter: Raphael Gaschignard | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: contrib.messages | Version: dev
Severity: Normal | Resolution:

Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Raphael Gaschignard):

* Attachment "cycle.png" added.

[Django]

#36662: django.contrib.messages Storage creates circular references with Request
objects
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: contrib.messages | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Raphael Gaschignard):

* Attachment "cycle.2.png" added.

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

[Django]

#36662: django.contrib.messages Storage creates circular references with Request
objects
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: contrib.messages | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Raphael Gaschignard):

* Attachment "cycle.png" removed.

[Django]

#36662: django.contrib.messages Storage creates circular references with Request
objects
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: contrib.messages | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Description changed by Raphael Gaschignard:

Old description:

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

New description:

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.

[[Image(cycle.2.png)]]

--
--
Ticket URL: <https://code.djangoproject.com/ticket/36662#comment:1>

[Django]

#36662: django.contrib.messages Storage creates circular references with Request
objects
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: contrib.messages | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Manish Tiwari):

Does someone working on this issue, or it's good to go for starting out
for this??
--
Ticket URL: <https://code.djangoproject.com/ticket/36662#comment:2>

[Django]

#36662: django.contrib.messages Storage creates circular references with Request
objects
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: contrib.messages | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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
--
Ticket URL: <https://code.djangoproject.com/ticket/36662#comment:3>

[Django]

#36662: django.contrib.messages Storage creates circular references with Request
objects
-------------------------------------+-------------------------------------

Reporter: Raphael Gaschignard | Owner: Augusto
Type: | Pontes
Cleanup/optimization | Status: assigned

Component: contrib.messages | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Augusto Pontes):

* owner: (none) => Augusto Pontes
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36662#comment:4>

[Django]

#36662: django.contrib.messages Storage creates circular references with Request
objects
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Augusto
Type: | Pontes
Cleanup/optimization | Status: assigned
Component: contrib.messages | Version: dev
Severity: Normal | Resolution:

Keywords: gc, garbage, | Triage Stage: Accepted
weakref |

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* keywords: => gc, garbage, weakref
* stage: Unreviewed => Accepted

Comment:

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

{{{#!diff
diff --git a/tests/messages_tests/base.py b/tests/messages_tests/base.py
index ce4b2acac8..b512788ec0 100644
--- a/tests/messages_tests/base.py
+++ b/tests/messages_tests/base.py
@@ -330,6 +330,22 @@ class BaseTests:
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,6 +356,15 @@ class BaseTests:
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()
}}}
{{{#!py
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:
{{{#!diff
diff --git a/django/contrib/messages/storage/base.py
b/django/contrib/messages/storage/base.py
index 5d89acfe69..9bca9e04c2 100644
--- a/django/contrib/messages/storage/base.py
+++ b/django/contrib/messages/storage/base.py
@@ -1,3 +1,4 @@
+import weakref
from django.conf import settings
from django.contrib.messages import constants, utils
from django.utils.functional import SimpleLazyObject
@@ -55,12 +56,16 @@ class BaseStorage:
"""

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:

{{{#!py
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36662#comment:5>

[Django]

#36662: django.contrib.messages Storage creates circular references with Request
objects
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Augusto
Type: | Pontes
Cleanup/optimization | Status: assigned
Component: contrib.messages | Version: dev
Severity: Normal | Resolution:
Keywords: gc, garbage, | Triage Stage: Accepted
weakref |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls):

Tenatively accepting on the premise that there is a non-awkward solution
for this given the way this app uses middleware.
--
Ticket URL: <https://code.djangoproject.com/ticket/36662#comment:6>

[Django]

#36662: django.contrib.messages Storage creates circular references with Request
objects
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Augusto
Type: | Pontes
Cleanup/optimization | Status: assigned
Component: contrib.messages | Version: dev
Severity: Normal | Resolution:
Keywords: gc, garbage, | Triage Stage: Accepted
weakref |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Augusto Pontes):

Replying to [comment:6 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/36662#comment:7>

[Django]

#36662: django.contrib.messages Storage creates circular references with Request
objects
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Augusto
Type: | Pontes
Cleanup/optimization | Status: assigned
Component: contrib.messages | Version: dev
Severity: Normal | Resolution:
Keywords: gc, garbage, | Triage Stage: Accepted
weakref |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls):

Sure, sharing any additional findings will be a help.
--
Ticket URL: <https://code.djangoproject.com/ticket/36662#comment:8>

[Django]

#36662: django.contrib.messages Storage creates circular references with Request
objects
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Augusto
Type: | Pontes
Cleanup/optimization | Status: assigned
Component: contrib.messages | Version: dev
Severity: Normal | Resolution:
Keywords: gc, garbage, | Triage Stage: Accepted
weakref |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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
--
Ticket URL: <https://code.djangoproject.com/ticket/36662#comment:9>