Code

Opened 8 months ago

Closed 8 months ago

Last modified 8 months ago

#20943 closed Bug (fixed)

Signal receivers cache should weakly reference senders.

Reported by: charettes 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)

0001-Fixed-20943-Weakly-reference-senders-when-caching-th.patch (4.9 KB) - added by charettes 8 months ago.

Download all attachments as: .zip

Change History (6)

comment:1 Changed 8 months ago by charettes

  • Severity changed from Release blocker to Normal
  • Summary changed from Signal receivers caching should weakly reference senders. to Signal receivers cache should weakly reference senders.

I guess this shouldn't be marked as Release Blocker since dynamic model creation is not usual among Django users.

comment:2 Changed 8 months ago by akaariai

  • Triage Stage changed from Unreviewed to 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?

Last edited 8 months ago by akaariai (previous) (diff)

comment:3 Changed 8 months ago by Simon Charette <charette.s@…>

  • Resolution set to fixed
  • Status changed from new to closed

In e55ca60903adcfd525938335b1ad9dbb6fd96c3e:

Fixed #20943 -- Weakly reference senders when caching their associated receivers

comment:4 Changed 8 months ago by Simon Charette <charette.s@…>

In f0bc2865ff9d85c952fa86ae19aee062a5e883cd:

Fixed #20943 -- Weakly reference senders when caching their associated receivers

Backport of e55ca60903 from master.

comment:5 Changed 8 months ago by Andrew Godwin <andrew@…>

In 63378163f9c5f3f9f9b42a6f260f798aa7e4b1f6:

Fixed #20943 -- Weakly reference senders when caching their associated receivers

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.