#20943 closed Bug (fixed)
Signal receivers cache should weakly reference senders.
Reported by: | Simon Charette | Owned by: | nobody |
---|---|---|---|
Component: | Core (Other) | Version: | 1.6-beta-1 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
The signal receivers caching introduced in 704ee33f503c96b96c2682b946a11b3b42318ba7 (#16679) to speed up model initialization keeps strong reference to sender thus preventing them from being garbage collected.
In [1]: import gc In [2]: import weakref In [3]: from django.dispatch import Signal In [4]: signal = Signal(use_caching=True) In [5]: def receiver(**kwargs): pass In [6]: signal.connect(receiver) In [7]: class cls: pass In [8]: wref = weakref.ref(cls) In [9]: signal.send(cls) Out[9]: [(<function __main__.receiver>, None)] In [10]: del cls In [11]: gc.collect() Out[11]: 33 In [12]: wref() is None Out[12]: False
Since the cache is only enabled on signals with BaseModel
((pre|post)_init
, (pre|post)_save
, (pre|post)_delete
and m2m_changed
) or module
(post_syncdb
) instances senders this shouldn't be an issue in usual cases since those objects are not meant to be garbage collected during the life of the application.
However it might cause issues to people dynamically creating model classes and modules since those objects won't be garbage collected as of Django 1.6.
I suggest we use a WeakKeyDictionary
to cache sender receivers in order to prevent regressions in 1.6 and avoid hard to traceback memory leaks.
Running djangobench on the soon to be attached patch revealed no significant slowdowns.
Attachments (1)
Change History (6)
by , 11 years ago
Attachment: | 0001-Fixed-20943-Weakly-reference-senders-when-caching-th.patch added |
---|
comment:1 by , 11 years ago
Severity: | Release blocker → Normal |
---|---|
Summary: | Signal receivers caching should weakly reference senders. → Signal receivers cache should weakly reference senders. |
comment:2 by , 11 years ago
Triage Stage: | Unreviewed → Ready for checkin |
---|
The patch looks good to me, and I can confirm that there isn't any performance regression in query_all test, which is dependant on model initialization signal speeds.
EDIT: just noticed, caching was removed from post_syncdb. Any reason for that?
comment:3 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
I guess this shouldn't be marked as Release Blocker since dynamic model creation is not usual among Django users.