[Django] #23641: Apps.set_installed_apps causes signals registered with apps as senders not to be received
Django
to be received
------------------------------+--------------------
Reporter: wrwrwr | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+--------------------
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.
[https://github.com/wrwrwr/django/tree/fix/missing-default-site-with-set-
installed-apps A branch] with an example test case and a possible partial
fix (that in full would be moving all module-level signal registration to
`AppConfig.ready`).
[https://github.com/wrwrwr/mezzanine/blob/maintenance/1.8-discover-test-
runner/mezzanine/utils/tests.py#L117 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23641>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
to be received
Reporter: wrwrwr | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
`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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23641#comment:1>
Django
to be received
Reporter: wrwrwr | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by wrwrwr):
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:
{{{#!python
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 [https://docs.djangoproject.com/en/dev/ref/signals/#management-
signals `pre/post_migrate/syncdb` documentation] (seems unlikely that
anyone would use an app config as a sender for any other signal)?
--
Ticket URL: <https://code.djangoproject.com/ticket/23641#comment:2>
Django
to be received
Reporter: wrwrwr | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by loic):
Somewhat transversal but do we still need `dispatch_uid` when registering
signals in `AppConfig.ready()`?
--
Ticket URL: <https://code.djangoproject.com/ticket/23641#comment:3>
Django
to be received
Reporter: wrwrwr | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
I believe it was mostly a workaround for the pre-1.4 project layout that
could result in duplicate imports.
--
Ticket URL: <https://code.djangoproject.com/ticket/23641#comment:4>
Django
to be received
Reporter: wrwrwr | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by wrwrwr):
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
[https://github.com/wrwrwr/django/compare/feature/testing-apps-with-
nonreentrant-ready 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).
--
Ticket URL: <https://code.djangoproject.com/ticket/23641#comment:5>
Django
to be received
Reporter: wrwrwr | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by wrwrwr):
* has_patch: 0 => 1
Old description:
> 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.
>
> [https://github.com/wrwrwr/django/tree/fix/missing-default-site-with-set-
> installed-apps A branch] with an example test case and a possible partial
> fix (that in full would be moving all module-level signal registration to
> `AppConfig.ready`).
>
> [https://github.com/wrwrwr/mezzanine/blob/maintenance/1.8-discover-test-
> runner/mezzanine/utils/tests.py#L117 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.
New description:
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.
[https://github.com/wrwrwr/django/compare/ticket_23641 A branch] with an
example test case and a possible fix.
[https://github.com/wrwrwr/mezzanine/blob/maintenance/1.8-discover-test-
runner/mezzanine/utils/tests.py#L117 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.
--
Comment:
Cleaned up and rebased
[https://github.com/wrwrwr/django/compare/ticket_23641 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23641#comment:6>
Django
to be received
Reporter: wrwrwr | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
Indeed, overriding `INSTALLED_APPS` re-runs the `ready()` methods of all
apps, so `dispatch_uid` is really useful in that case.
--
Ticket URL: <https://code.djangoproject.com/ticket/23641#comment:7>
Django
to be received
Reporter: wrwrwr | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
@wrwrwr, I created a [https://github.com/django/django/pull/3426 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!
--
Ticket URL: <https://code.djangoproject.com/ticket/23641#comment:8>
Django
to be received
Reporter: wrwrwr | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by wrwrwr):
* needs_better_patch: 1 => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23641#comment:9>
Django
to be received
Reporter: wrwrwr | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23641#comment:10>
Django
to be received
Reporter: wrwrwr | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by wrwrwr):
* needs_better_patch: 1 => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23641#comment:11>
Django
to be received
Reporter: wrwrwr | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"d66bda60590daabe21f60a532a613a31a10fedbd"]:
{{{
#!CommitTicketReference repository=""
revision="d66bda60590daabe21f60a532a613a31a10fedbd"
Added notes on registering signals in ready() and using dispatch_uid.
Refs #23641.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23641#comment:12>
Django
to be received
Reporter: wrwrwr | Owner: nobody
Type: Bug | Status: closed
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"dd35cc232aefd491c9b3b4461a22055734daf376"]:
{{{
#!CommitTicketReference repository=""
revision="dd35cc232aefd491c9b3b4461a22055734daf376"
Fixed #23641 -- Moved post_migrate signals for contrib apps to
AppConfig.ready().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23641#comment:13>
Django
to be received
Reporter: wrwrwr | Owner: nobody
Type: Bug | Status: closed
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"3ac39bb63d6155c784690394d27bbdfc4b80f957"]:
{{{
#!CommitTicketReference repository=""
revision="3ac39bb63d6155c784690394d27bbdfc4b80f957"
[1.7.x] Added notes on registering signals in ready() and using
dispatch_uid.
Refs #23641.
Backport of d66bda60590daabe21f60a532a613a31a10fedbd from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23641#comment:14>
Django
to be received
Reporter: wrwrwr | Owner: nobody
Type: Bug | Status: closed
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23641#comment:15>