post_delete signal fires before objects are removed from database

[hibiscuits]

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.

[Simon Charette]

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?

[hibiscuits]

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. By the way, I appreciate you not closing having closed this immediately.

[Simon Charette]

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.

[hibiscuits]

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