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.