Opened 5 years ago

Closed 5 years ago

#30835 closed Cleanup/optimization (invalid)

post_delete signal fires before objects are removed from database

Reported by: hibiscuits Owned by: nobody
Component: Documentation Version: 2.2
Severity: Normal Keywords: post_delete signal transaction on_commit
Cc: Simon Charette Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (5)

comment:1 by Simon Charette, 5 years ago

Cc: Simon Charette added

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?

in reply to:  1 comment:2 by hibiscuits, 5 years ago

Replying to 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.

Version 0, edited 5 years ago by hibiscuits (next)

comment:3 by Simon Charette, 5 years ago

Component: Database layer (models, ORM)Documentation
Easy pickings: unset
Keywords: transaction on_commit added
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

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

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.

in reply to:  3 comment:4 by hibiscuits, 5 years ago

Replying to 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

comment:5 by hibiscuits, 5 years ago

Resolution: invalid
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top