Apps.set_installed_apps causes signals registered with apps as senders not to be received

[Wojtek Ruszczewski]

Calling Apps.set_installed_apps before DiscoverRunner.setup_databases causes pre/post_migrate signal handlers registered on module import with app configs as senders not to be executed for the test database.

The signal dispatcher stores id() of the sender argument provided to connect to identify the sender that we're interested in receiving the signals from, in case of post_migrate signals this is often an id of an AppConfig for the registering app. When set_installed_apps is called it reinstantiates all app configs. If afterwards you send a signal using app config from the global app registry as the sender (as is the case in emit_post_migrate_signal when setting up test databases), its id won't match the one stored by the dispatcher, thus signal handlers won't get executed.

​A branch with an example test case and a possible fix.

​A sketch of a possible more general solution / workaround (this Mezzanine wrapper for DiscoverRunner turned out pretty involved, you're welcome to suggest a cleaner implementation :-)

See also #22688: AppConfig.ready is going to get called for every instance, so should be a good place to put these signal registrations.

[Aymeric Augustin]

set_installed_apps is a private API required mainly to test Django itself and I don't think we can make it work reliably in general.

That said, moving signal registration code to AppConfig.ready() looks like a better design in general. Accepting the ticket on this basis.

[Wojtek Ruszczewski]

Thanks for the quick response. Good news is that scanning the source for ".connect(", "@receiver(" and "sender=" didn't reveal any other cases of using AppConfig as a sender, so for consistency I've only also moved post_migrate registration for auth and contenttypes to ready (both need to be called for every migration, not just their app migration, so don't specify any sender to connect).

On the other hand, theoretically the issue can be triggered without directly using set_installed_apps; the following test fails as is, but succeeds if you comment out the modify_settings decorator:

from django.apps import apps
from django.dispatch import Signal, receiver
from django.test import TestCase, modify_settings

some_signal = Signal()
app_label = 'auth'  # Something installed.
handler_called = False

@receiver(some_signal, sender=apps.get_app_config(app_label))
def signal_handler(sender, **kwargs):
    global handler_called
    handler_called = True

@modify_settings(INSTALLED_APPS={})
class Test(TestCase):
    def test_handler(self):
        some_signal.send(apps.get_app_config(app_label))
        self.assertTrue(handler_called)

This is probably an edge case no one will ever run into, nevertheless the behavior may be somewhat surprising.

How about adding a note to the docs, say something like:

If you provide an AppConfig instance as the sender argument, please ensure that the signal is registered in AppConfig.ready as app configs can be reinstantiated during testing.

at the ​`pre/post_migrate/syncdb` documentation (seems unlikely that anyone would use an app config as a sender for any other signal)?

[Loic Bistuer]

Somewhat transversal but do we still need dispatch_uid when registering signals in AppConfig.ready()?

[Aymeric Augustin]

I believe it was mostly a workaround for the pre-1.4 project layout that could result in duplicate imports.

[Wojtek Ruszczewski]

I'd leave it, it seems the double registration is even more likely when connecting signals within ready than on the module level. If modify_settings with installed apps is used to wrap a test ready is reexecuted, so without the dispatach_uid, if someone were to send any post_migrate signal from a test the handler would be processed twice. Maybe that should also be mentioned in the docs:

(continuing) Otherwise consider providing dispatch_uid to avoid connecting your signals more than once when ready is reexecuted.

That is unless we'd avoid calling ready more than once, something like ​this should do it (I'm aware you've decided against this in #22688 just a few months ago, sorry for bringing it back here).

[Wojtek Ruszczewski]

Cleaned up and rebased ​branch moving the post_migrate signal registration for auth, contenttypes and sites to ready and adding two notes to the docs (in short: -- registering signals in ready may require a dispatch_uid, -- signals using app config as a sender should only be registered in ready). I may be overdoing the documentation, so please consider if it wouldn't be better to just skip the notes.

[Aymeric Augustin]

Indeed, overriding INSTALLED_APPS re-runs the ready() methods of all apps, so dispatch_uid is really useful in that case.

[Tim Graham]

@wrwrwr, I created a ​PR from your branch and left a couple comments. Could you please uncheck "Patch needs improvement" when you have a chance to update it? Thanks!

[Wojtek Ruszczewski]

Thanks for the review and taking care of this. I believe those test_group_permission_performance failures are not directly related to the signal registration changes, so left them unfixed here and created #23746 with a potential solution. Let me know, if you'd prefer to avoid such tickets interdependence -- we may add a cache_clear hot-fix here and decide whether to revert it or not later.

[Tim Graham]

Even if the root cause of the failures is not directly related to this, we cannot commit this until those failures are worked around in this patch or until the related ticket is resolved. Currently our build doesn't have these failures on a regular basis.

[Wojtek Ruszczewski]

Added the hotfix for the test_group_permission_performance and rebased the pull. These changes don't look so "heavy" as one or another cache clearing solution, so a simple, specific fix may be in place.

[Tim Graham <timograham@…>]

In d66bda60590daabe21f60a532a613a31a10fedbd:

Added notes on registering signals in ready() and using dispatch_uid.

Refs #23641.

[Tim Graham <timograham@…>]

In dd35cc232aefd491c9b3b4461a22055734daf376:

Fixed #23641 -- Moved post_migrate signals for contrib apps to AppConfig.ready().

[Tim Graham <timograham@…>]

In 3ac39bb63d6155c784690394d27bbdfc4b80f957:

[1.7.x] Added notes on registering signals in ready() and using dispatch_uid.

Refs #23641.

Backport of d66bda60590daabe21f60a532a613a31a10fedbd from master

[Tim Graham]

Putting the original commit message here for posterity:

In the case of the sites app, this solves a problem appearing when
Apps.set_installed_apps() (override/modify_settings) is used before
sending the post_migrate signal -- create_default_site() handler is
not executed in such a case.

To elaborate a bit, the signal is connected with the sender attribute
equal to an AppConfig instance; the signal dispatcher stores its id to
identify the sender that we're interested in receiving signals from.
The set_installed_apps() method reinstantiates all AppConfigs, and
when the migration machinery emits signals using these new app configs
as senders, their ids don't match those stored by the dispatcher.
Handlers registered before overriding installed apps are not called.

Moving signal registration to ready() ensures that signals that use
an app config as a sender will be connected for any new instance.
On the other hand, the ready() method may be executed more than once
during testing, so signals that expect other senders may need to be
protected from double registration.