#18094: signals, model inheritance, and proxy models
Carl Meyer
There's a discussion ongoing on ticket #18094
(https://code.djangoproject.com/ticket/18094) that has enough potential
back-compat implications that it seems worth getting feedback here.
Currently, when you delete a concrete-inheritance child model instance,
pre_delete and post_delete signals are sent both for the child and
parent models.
In contrast, when you delete a proxy model instance, the pre/post_delete
signals are sent only for the proxy model class, never for the parent
concrete class.
This is problematic. Imagine a reusable tagging app that has a Tag
model, and attaches a pre_delete signal handler for Tag instances. In my
usage of that app, I use my own TagProxy subclass. When I delete a
TagProxy instance, a Tag is in fact deleted, but the app's pre_delete
signal handler never fires. This violates the reasonable assumption that
subclassing doesn't change superclass behavior except where it
explicitly overrides it.
So we'd like to make the obvious fix, which is to fire the
pre/post_delete signals for every superclass of the instance(s) you
delete, regardless of whether they are parents via concrete or proxy
inheritance.
This raises the question of consistency with pre/post_save and
pre/post_init, which are currently both sent only once, for the exact
class being saved/initialized (thus already inconsistent with delete
signals, which are also sent for concrete-inheritance parents). The same
Tag scenario above would apply equally to save and init signals: the
tagging app should be able to assume that if it registers a save or init
signal handler for Tag, it will get called whenever a Tag is saved or
initialized, even if that's happening via a Tag subclass.
So it seems that perhaps we should also fix the save and init signals to
be fired for each superclass. Is this an acceptable change from a
backwards-compatibility perspective? In the common case of registering a
signal handler using "sender=", it should be a clear win: signal
handlers will now execute when you'd expect them to. But in the case of,
say, a generic audit-trail handler that listens for all post_save (no
sender specified), it will now get what could seem like duplicate
signals for inherited models (one for each superclass of the instance
being saved).
Thoughts?
(The other approach that might come to mind for fixing this would be to
move the fix into the signals framework itself, so that it executes
signal handlers registered for signal=Foo anytime a signal is fired for
anything that passes issubclass(Foo), rather than requiring Foo
identity. This is appealing, and would solve the potential "duplicate
signals" problem with a generic receiver, but is a much more invasive
change in the fundamental semantics of the signals framework, so I don't
think it's a realistic option.)
Carl
Jeremy Dunck
> Hi all,
>
> There's a discussion ongoing on ticket #18094
> (https://code.djangoproject.com/ticket/18094) that has enough potential
> back-compat implications that it seems worth getting feedback here.
Small note, I'll try to respond to the whole thing later. This ticket
exists on the topic, too:
https://code.djangoproject.com/ticket/9318
Javier Guerra Giraldez
> Thoughts?
in my mental model, there are three types of inheritance:
concrete: two tables, deletion should fire two signals, one for the
child record and one for the parent record.
abstract: there's no parent table, deletion should fire one signal,
for the child record; unless the parent overrides the delete() method
and chooses to send a custom signal
proxy: there's no child table. kind of an 'abstract child', instead
of 'abstract parent'. deletion should fire one signal, for the
_parent_ record (which is the only real record), unless the child
overrides the delete() method and chooses to send a custom signal.
IOW, i think the existing signals are database-related and should be
fired only for the concrete part(s). if the abstract part wants to,
it can send custom signals.
second idea:
define three different signals: one for concrete members, one for
abstract, and one for both.
- non-inheritance deletion:
- send concrete-deleted and common-deleted
- concrete inheritance deletion (both are concrete):
- child: concrete-deleted and common-deleted
- parent: concrete-deleted and common-deleted
- abstract inheritance (parent is abstract):
- child: concrete-deleted and common-deleted
- parent: abstract-deleted and common-deleted
- proxy inheritance (child is abstract):
- child: abstract-deleted and common-deleted
- parent: concrete-deleted and common-deleted
but i think that's overkill
--
Javier
Carl Meyer
Thanks for pointing that out! I've closed #18094 as a duplicate of
#9318, as they really are addressing the same issue. #9318 is currently
taking the approach I'd dismissed as "not a realistic option" of fixing
the signals framework itself to be smart about sender subclasses. It may
be that I dismissed that option too quickly.
Carl
Carl Meyer
> in my mental model, there are three types of inheritance:
>
> concrete: two tables, deletion should fire two signals, one for the
> child record and one for the parent record.
>
> abstract: there's no parent table, deletion should fire one signal,
> for the child record; unless the parent overrides the delete() method
> and chooses to send a custom signal
>
> proxy: there's no child table. kind of an 'abstract child', instead
> of 'abstract parent'. deletion should fire one signal, for the
> _parent_ record (which is the only real record), unless the child
> overrides the delete() method and chooses to send a custom signal.
>
>
> IOW, i think the existing signals are database-related and should be
> fired only for the concrete part(s). if the abstract part wants to,
> it can send custom signals.
I agree that signals should not be fired for abstract models. Proxy
models are a rather different case, though - they exist precisely in
order to be able to augment/supplement Python-level model behavior, and
signals are a part of that behavior. It would seem unfortunate to me if
you couldn't attach signal handlers specifically to a proxy subclass of
a model.
Also, firing signals only for the concrete superclass of a proxy model
would be a much worse backwards-incompatible change than any of the
others discussed. Signal handlers (for delete, save, or init) registered
for a proxy model class specifically currently work - with this change
they would just suddenly stop firing.
> second idea:
>
> define three different signals: one for concrete members, one for
> abstract, and one for both.
>
> - non-inheritance deletion:
> - send concrete-deleted and common-deleted
>
> - concrete inheritance deletion (both are concrete):
> - child: concrete-deleted and common-deleted
> - parent: concrete-deleted and common-deleted
>
> - abstract inheritance (parent is abstract):
> - child: concrete-deleted and common-deleted
> - parent: abstract-deleted and common-deleted
>
> - proxy inheritance (child is abstract):
> - child: abstract-deleted and common-deleted
> - parent: concrete-deleted and common-deleted
>
> but i think that's overkill
Yes, I think this is too much complexity compared to the other solutions.
Carl
Carl Meyer
> fired only for the concrete part(s). if the abstract part wants to,
> it can send custom signals.
Also, it isn't really true that the model signals are strictly tied to
database activity; they are tied to events on Python model objects. One
of the three signals under discussion is the pre/post_init signal, which
fires anytime a model instance is instantiated, even if no database
query or database change occurred.
Carl
Javier Guerra Giraldez
> Also, it isn't really true that the model signals are strictly tied to
> database activity; they are tied to events on Python model objects. One
> of the three signals under discussion is the pre/post_init signal, which
> fires anytime a model instance is instantiated, even if no database
> query or database change occurred.
i think this is the semantic issue that should be explicit:
if those signals are for _every_ model, then should fire for abstract
parents and abstract children (proxy models).
maybe a 'fire_signals' meta option that defaults to False on abstract
parents and True on proxy (and normal) models?
--
Javier
Anssi Kääriäinen
than they currently are. Even now they can give around 100% overhead
to model.__init__(). And this is in a case where the currently
initialized model has no signals attached at all - it is enough that
_some_ model in the project has pre or post init signals attached. I
don't believe signaling cost is severe for .save and .delete as those
operations are doing much heavier work than __init__ anyways, but for
init the cost is big enough already. If at all possible, make them
cheaper.
In addition, just changing the signals to fire for every parent class
is pretty much as backwards incompatible as it gets: imagine you have
post_save signal which increments a counter in a related model on
create (posts in a thread for example). You have also a proxy model,
and you have attached the same signal to the proxy model, too, because
that is what you have to do to get the signal fire when you save the
proxy model. After the change you will get the counter incremented two
times for create, and you have a data corrupting bug. This might not
be a common pattern, but for example Django's use of pre and post init
signals follows the pattern of attaching to each subclass individually
and these signals would thus fire multiple times.
- Anssi
Carl Meyer
> It is important that pre/post init signals will not get more expensive
> than they currently are. Even now they can give around 100% overhead
> to model.__init__(). And this is in a case where the currently
> initialized model has no signals attached at all - it is enough that
> _some_ model in the project has pre or post init signals attached. I
> don't believe signaling cost is severe for .save and .delete as those
> operations are doing much heavier work than __init__ anyways, but for
> init the cost is big enough already. If at all possible, make them
> cheaper.
So this is an argument against firing the init signals multiple times,
for each superclass, but it's not an argument against changing the
signal framework to include superclass receivers, as proposed in #9318;
that would not change the performance characteristics for the
no-receivers situation.
(I see that this choice also has implications for the performance
improvement patch in #16679, but it looks like we have a workable option
there either way).
> In addition, just changing the signals to fire for every parent class
> is pretty much as backwards incompatible as it gets: imagine you have
> post_save signal which increments a counter in a related model on
> create (posts in a thread for example). You have also a proxy model,
> and you have attached the same signal to the proxy model, too, because
> that is what you have to do to get the signal fire when you save the
> proxy model. After the change you will get the counter incremented two
> times for create, and you have a data corrupting bug. This might not
> be a common pattern, but for example Django's use of pre and post init
> signals follows the pattern of attaching to each subclass individually
> and these signals would thus fire multiple times.
Yes, this is similar to the audit-trail handler I mentioned, and I agree
that it is a significant backwards-compatibility problem for either option.
So perhaps we do need the signal inheritance behavior to be opt-in when
connecting the signal handler. I think I'd like to see a deprecation
path so that eventually the inheritance behavior is the default, and you
have to opt out of it, as I think in most cases it's the desired behavior.
Carl
Alex Ogier
I think changing when signals fire is bound to cause breakages for some apps, and of the worst variety because signals both deal with basic data integrity and are relatively opaque (I.e. debugging is a pain). Even if the current behavior isn't what we would choose given a blank slate, it can't realistically be changed.
On the other hand, supporting thus use case isn't impossible. Maybe we could add a "fire_for_subclasses" kwarg to the connect() method or something. That way concrete models can specify that they absolutely want their signals fired, and the resolution can be done at load time to avoid performance hits from isinstance calls.
Best,
Alex Ogier
Anssi Kääriäinen
> So this is an argument against firing the init signals multiple times,
> for each superclass, but it's not an argument against changing the
> signal framework to include superclass receivers, as proposed in #9318;
> that would not change the performance characteristics for the
> no-receivers situation.
>
> (I see that this choice also has implications for the performance
> improvement patch in #16679, but it looks like we have a workable option
> there either way).
inheritance there, and you could then add the signals directly to
model._meta.pre/post_init_signals. Speed would improve, and as far as
I understand these signals are not needed for anything else than
fields doing special setup for related variables (ImageField setting
width and height for example).
Of course, if the receivers caching from #16679 would get in then the
init signals wouldn't be a speed problem. But I would hope that #16679
would not be needed at all if init signals were handled specially, as
other signals do not have any meaningful overhead (warning: haven't
benchmarked). #16679 is making a complex piece of code even more
complex.
> Yes, this is similar to the audit-trail handler I mentioned, and I agree
> that it is a significant backwards-compatibility problem for either option.
>
> So perhaps we do need the signal inheritance behavior to be opt-in when
> connecting the signal handler. I think I'd like to see a deprecation
> path so that eventually the inheritance behavior is the default, and you
> have to opt out of it, as I think in most cases it's the desired behavior.
forward. I don't know how to do the technical details... from
__future__ import pre_save? :)
- Anssi
Anssi Kääriäinen
> > So perhaps we do need the signal inheritance behavior to be opt-in when
> > connecting the signal handler. I think I'd like to see a deprecation
> > path so that eventually the inheritance behavior is the default, and you
> > have to opt out of it, as I think in most cases it's the desired behavior.
>
> Unfortunately this seems to be the only backwards compatible way
> forward. I don't know how to do the technical details... from
> __future__ import pre_save? :)
when .connect() is called? The inherit mode must be one of True,False
or None. Default of None means no inheritance (as now) but it will be
deprecated.
Another problem is that delete fires the same signal multiple times in
the inheritance chain. We can't remove the parent signal for backwards
compatibility reasons, nor can we fire them as then inheriting
listeners will see the delete signals multiple times. Maybe the parent
class signal could be sent in a way that it is only visible to those
listeners having None as their inherit argument - that is those who
use the deprecation mode setting. When the None as argument is
removed, so is the parent delete signal, too.
The above would lead into clean situation where signals are handled
coherently. I know the above is ugly, but it is just for the
transition period.
Another option is to continue as currently: model signals aren't
handled coherently, and there is no signal inheritance. Django users
have managed to survive thus far with the current signals
implementation, so maybe it doesn't need fixing?
Any opinions on the above transition phase idea? Other ideas? Is the
"define signal inheritance on connect" the wanted behavior in 1.7 or
should it be something else?
- Anssi
Jeremy Dunck
<anssi.ka...@thl.fi> wrote:
> On Apr 12, 10:27 pm, Anssi Kääriäinen <anssi.kaariai...@thl.fi> wrote:
>> > So perhaps we do need the signal inheritance behavior to be opt-in when
>> > connecting the signal handler. I think I'd like to see a deprecation
>> > path so that eventually the inheritance behavior is the default, and you
>> > have to opt out of it, as I think in most cases it's the desired behavior.
>>
>> Unfortunately this seems to be the only backwards compatible way
>> forward. I don't know how to do the technical details... from
>> __future__ import pre_save? :)
>
> Could we force the caller to define the wanted signal inheritance mode
> when .connect() is called? The inherit mode must be one of True,False
> or None. Default of None means no inheritance (as now) but it will be
> deprecated.
https://github.com/votizen/django/tree/fe0f92b0f50bcd47742638ff3b8ff94af83ff9c5
(Tracking branch: https://github.com/votizen/django/tree/virtual_signals )
I haven't tried to address performance yet, but I've read all of
https://code.djangoproject.com/ticket/16679 and think it's possible to
keep sane overhead.
> Another problem is that delete fires the same signal multiple times in
> the inheritance chain. We can't remove the parent signal for backwards
> compatibility reasons, nor can we fire them as then inheriting
> listeners will see the delete signals multiple times.
https://github.com/votizen/django/blob/270666cc701dff1eb1a8e5e649634aee5980b737/django/dispatch/dispatcher.py#L249
Basically we can group up the receivers so that they are fired at most once.
...
> handled coherently, and there is no signal inheritance. Django users
> have managed to survive thus far with the current signals
> implementation, so maybe it doesn't need fixing?
of reasoning is wrong - mostly people don't notice when their signal
handlers are doing the wrong thing. The lack of visibility makes it
difficult. I have definitely seen bugs caused by this misbehavior.
(I'm not in favor of removing signals entirely, for what it's worth -
I still don't see a better way in some cases - but I do think more
visibility would be useful.)
> Any opinions on the above transition phase idea? Other ideas? Is the
> "define signal inheritance on connect" the wanted behavior in 1.7 or
> should it be something else?
"1.7" here. You mean 1.5 or done with the deprecation path by 1.7?
Anssi Kääriäinen
> On Fri, Apr 20, 2012 at 9:01 AM, Anssi Kääriäinen
> > when .connect() is called? The inherit mode must be one of True,False
> > or None. Default of None means no inheritance (as now) but it will be
> > deprecated.
>
> (Tracking branch:https://github.com/votizen/django/tree/virtual_signals)
>
> I haven't tried to address performance yet, but I've read all ofhttps://code.djangoproject.com/ticket/16679and think it's possible to
> keep sane overhead.
There is one two really important signals for performance: pre_init
and post_init. If these are fast (maybe by special casing), then the
rest of the signals wont be that important. IIRC the caching approach
will give you good performance assuming there aren't any signals for
the model which is inited. This isn't true currently. Of course, all
bets are off if there are pre/post init signals for the model. We need
to either make the pre/post init signals fast enough, or make some
special casing for them.
> > Any opinions on the above transition phase idea? Other ideas? Is the
> > "define signal inheritance on connect" the wanted behavior in 1.7 or
> > should it be something else?
>
> I think this is the right approach - but I'm not sure why you said
> "1.7" here. You mean 1.5 or done with the deprecation path by 1.7?
to ask about the default behavior in 1.7 forward (inherit or no-
inherit) but just wrote something different. Hard to know now...
- Anssi