Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ConfigWatcher needs to register receivers using strong references #495

Merged
merged 1 commit into from
Nov 6, 2023
Merged

fix: ConfigWatcher needs to register receivers using strong references #495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ Change Log
Unreleased
~~~~~~~~~~

[3.1.1] - 2023-11-06
~~~~~~~~~~~~~~~~~~~~
Fixed
_____
* ConfigWatcher should now respond to model events properly now that it registers receivers with strong references. (Tested in sandbox.)

[3.1.0] - 2023-10-31
~~~~~~~~~~~~~~~~~~~~

Expand Down
2 changes: 1 addition & 1 deletion edx_arch_experiments/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
A plugin to include applications under development by the architecture team at 2U.
"""

__version__ = '3.1.0'
__version__ = '3.1.1'
13 changes: 11 additions & 2 deletions edx_arch_experiments/config_watcher/signals/receivers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,24 @@ def _register_waffle_observation(*, model, short_name, fields):
short_name (str): A short descriptive name for an instance of the model, e.g. "flag"
fields (list): Names of fields to report on in the Slack message
"""
@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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, do you know why it stores weak references and what type of clean-up problems this might cause?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# functions would then be garbage-collected, and Django would drop
# them. So pass weak=False to make Django hold strong references
# instead. (It works either way in devstack, apparently due to an
# interaction with settings.DEBUG causing a reference to be held:
# <https://stackoverflow.com/a/70651310>.)

@receiver(signals.post_save, sender=model, weak=False, dispatch_uid=f"config_watcher_{short_name}_change")
def report_waffle_change(*args, instance, created, **kwargs):
try:
_report_waffle_change(short_name, instance, created, fields)
except: # noqa pylint: disable=bare-except
# Log and suppress error so Waffle change can proceed
log.exception(f"Failed to report change to waffle {short_name}")

@receiver(signals.post_delete, sender=model, dispatch_uid=f"config_watcher_{short_name}_delete")
@receiver(signals.post_delete, sender=model, weak=False, dispatch_uid=f"config_watcher_{short_name}_delete")
def report_waffle_delete(*args, instance, **kwargs):
try:
_report_waffle_delete(short_name, instance)
Expand Down