[Django] #30835: post_delete signal fires before objects are removed from database
Django
-------------------------------------+-------------------------------------
Reporter: | Owner: nobody
hibiscuits |
Type: Bug | Status: new
Component: Database | Version: 2.2
layer (models, ORM) |
Severity: Normal | Keywords: post_delete signal
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-------------------------------------+-------------------------------------
This was brought up by someone else in
https://code.djangoproject.com/ticket/29319, but it doesn't look like it
was fully understood.
In the delete() method at the bottom of db/models/deletion.py, the
post_delete signal is sent ''inside'' the atomic transaction block. This
means that if the transaction subsequently fails for whatever reason, the
post_delete signal has already been sent, but the object remains in the
database. A simple fix is to move that code outside of the transaction
block, so that the deletion will have been transacted without error in
order for the post_delete signal to be sent.
--
Ticket URL: <https://code.djangoproject.com/ticket/30835>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: post_delete signal | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Simon Charette (added)
Comment:
> This means that if the transaction subsequently fails for whatever
reason, the post_delete signal has already been sent, but the object
remains in the database.
I'm not sure I'm following you here. The `post_delete` signal is
explicitly sent within the same transaction as the actual deletion to
ensure database changes performed in the receivers are tied to the
transaction. In short that means that if something causing the transaction
to fail in the receivers happens everything will be rolledback instead of
leaving dangling changes in the database.
If you need to perform changes once the deletion of objects is committed
you should use `transaction.on_commit` to defer their execution.
https://docs.djangoproject.com/en/2.2/topics/db/transactions/#performing-
actions-after-commit
Just to make sure I fully understand your report before closing this
ticket, could you provide an example of your theory?
--
Ticket URL: <https://code.djangoproject.com/ticket/30835#comment:1>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by hibiscuits):
Replying to [comment:1 Simon Charette]:
> The post_delete signal is explicitly sent within the same transaction as
the actual deletion to ensure database changes performed in the receivers
are tied to the transaction.
Ah. I now understand why it is written the way it is. But it is
certainly not clear from the documentation that the {{{post_delete}}}
signal is suitable only for other database activities and is unsuitable
for things like sending a notification. I do think that with a name like
{{{post_delete}}}, it would be natural to assume that the deletion had
actually been committed before sending the signal. In fact, in the docs
for {{{post_delete}}}, it explicitly says that the instance passed ''will
not exist in the database'' at that point. Perhaps there should be a
{{{post_deletion_commit}}} signal. The {{{on_commit}}} method you linked
to looks like it would do the trick, but the whole appeal of a signal is
that you don't have to write any logic in the place where the action is
actually called.
> Just to make sure I fully understand your report before closing this
ticket, could you provide an example of your theory?
I was trying to use it to send a notification. So instance.delete() was
called, a foreign key restraint failed, the transaction rolled back, and
the deletion notification had already been sent. I worked around it by
writing my own signal and sending it after instance.delete() returned.
So, in summary, I understand your reasoning, but it seems there should be
another signal at best, and a doc change at a minimum.
--
Ticket URL: <https://code.djangoproject.com/ticket/30835#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: 2.2
Severity: Normal | Resolution:
Keywords: post_delete signal | Triage Stage: Accepted
transaction on_commit |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Simon Charette):
* keywords: post_delete signal => post_delete signal transaction on_commit
* type: Bug => Cleanup/optimization
* component: Database layer (models, ORM) => Documentation
* easy: 1 => 0
* stage: Unreviewed => Accepted
Comment:
Thank you for taking the time to detail your reasoning.
I personally find the `on_commit` documentation clear about this matter
but it's true that it's no easily discoverable from the `post_delete`
documentation. I'll let other developers chime in about whether or not
that would be worth it. If you give it a shot to propose a clearer
admonition that would certainly help in deciding whether or not this
avenue should be pursued.
Regarding the `post_delete_commit` signal I'm not convinced it would be
worth adding to core. It can easily be implemented by connecting a
`post_delete` signal receiver that defers a `send` on
`transaction.on_commit`
e.g
{{{#!python
from django.dispatch import Signal
from django.db.models.signals import post_delete
post_delete_commit = Signal(post_delete.providing_args)
def send_post_delete_commit(*args, **kwargs):
kwargs['signal'] = post_delete_commit
transaction.on_commit(partial(post_delete_commit.send, *args,
**kwargs), using=kwargs['using'])
post_delete.connect(send_post_delete_commit)
}}}
Note that this will have the unfortunate side effect of disabling fast-
delete for all models though which is not something to consider lightly.
That fact that connecting deletion signals turns fast-delete off is also
something that should be documented.
--
Ticket URL: <https://code.djangoproject.com/ticket/30835#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: 2.2
Severity: Normal | Resolution:
Keywords: post_delete signal | Triage Stage: Accepted
transaction on_commit |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by hibiscuits):
Replying to [comment:3 Simon Charette]:
> I personally find the on_commit documentation clear about this matter
but it's true that it's no easily discoverable from the post_delete
documentation.
Agreed.
> Regarding the post_delete_commit signal I'm not convinced it would be
worth adding to core. It can easily be implemented by connecting a
post_delete signal receiver that defers a send on transaction.on_commit
I realized this in the moments after I wrote my comment and went back and
deleted that part, but I was too slow!
I will open a separate issue for the documentation stuff, and I now agree
that nothing needs to change in the code.
Cheers
--
Ticket URL: <https://code.djangoproject.com/ticket/30835#comment:4>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Documentation | Version: 2.2
Keywords: post_delete signal | Triage Stage: Accepted
transaction on_commit |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => invalid
--
Ticket URL: <https://code.djangoproject.com/ticket/30835#comment:5>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Documentation | Version: 2.2
Severity: Normal | Resolution: invalid
Keywords: post_delete signal | Triage Stage: Accepted
transaction on_commit |
-------------------------------------+-------------------------------------
#26630 was a dupe. If there's a proposed docs tweak for `post_delete,` we
can reopen & triage it.
--
Ticket URL: <https://code.djangoproject.com/ticket/30835#comment:6>