fix: ConfigWatcher needs to register receivers using strong references #495
Conversation
These inner functions would otherwise get GC'd right away, since Django only holds weak references to receivers by default.
timmc-edx
force-pushed
the
timmc/strong
branch
from
af32b75
to
88dab7a
Compare
| @receiver(signals.post_save, sender=model, dispatch_uid=f"config_watcher_{short_name}_change") | ||
|
|
||
| # Note that weak=False is required here. Django by default only | ||
| # holds weak references to receiver functions. But these inner |
There was a problem hiding this comment.
Out of curiosity, do you know why it stores weak references and what type of clean-up problems this might cause?
There was a problem hiding this comment.
I'm not sure why it stores weak references, no. I could make some guesses -- perhaps there are architectures where signals are added temporarily, or where an object listens to signals during its lifetime but those listeners should not block garbage collection of the object.
I don't foresee any issue with using strong references here. Normally our @receiver-decorated functions are top-level functions, which means the enclosing module would hold a strong reference to the function.
I don't think plugin signals would have had this issue because they require you to point to a top-level function. (Which is also why they were unsuitable for this app, at least if we want to use sender. The alternative would be to listen to all model saves in a top-level function and then have a conditional in the receiver's body.)
There was a problem hiding this comment.
Incidentally, I actually had a branch up for a bit with the top-level function approach before I realized I had overlooked the weak param: main...timmc/top-level#diff-1206f94d4250bda1d46a290ece9b20f6bc8b2f961b6f9adfc91a42002d54099aR93
These inner functions would otherwise get GC'd right away, since Django only holds weak references to receivers by default. (I don't know why this worked in devstack.)
Merge checklist:
Check off if complete or not applicable: