#33616 closed New feature (fixed)
Supporting robust on_commit handlers.
Reported by: | Josh Smeaton | Owned by: | Abhinav Yadav |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 4.0 |
Severity: | Normal | Keywords: | |
Cc: | Adam Johnson, David Wobrock | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description (last modified by )
I recently tracked down an issue in my application where some on_commit handlers didn't execute because one of the previous handlers raised an exception. There appears to be no way to execute on_commit handlers *robustly* as you're able to do with signals [0] using send_robust.
I could sprinkle try/catches around the place, but I'd like to avoid doing so because not all functions that are used as handlers should always swallow exceptions, but could do so when run as on_commit handlers.
Targeting which handlers can be robust or not would be really useful, for example:
def update_search(user): # if updating search fails, it's fine, we'll bulk update later anyway transaction.on_commit(lambda: search.update(user), robust=True) def trigger_background_task_one(user): # if this task fails, we want to crash transaction.on_commit(lambda: mytask.delay(user_id=user.id))
Here if search fails to update it doesn't prevent the background task from being scheduled.
I'm proposing to add a robust
kwarg that defaults to False, for backward compatibility, but allows a user to tag specific handlers as such.
[0] https://docs.djangoproject.com/en/4.0/topics/signals/#sending-signals
Change History (21)
comment:1 by , 3 years ago
Description: | modified (diff) |
---|
comment:2 by , 3 years ago
Cc: | added |
---|---|
Component: | Uncategorized → Database layer (models, ORM) |
Summary: | Supporting robust on_commit handlers → Supporting robust on_commit handlers. |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → New feature |
comment:4 by , 3 years ago
I haven't got the time to put a patch together *right now* but I could do so in the near future. Consider tagging this as "easy pickings" for a budding contributor?
comment:5 by , 3 years ago
Easy pickings: | set |
---|
comment:6 by , 3 years ago
I've started an easy pickings thread on -developers ML and would be happy to review and guide someone to make the change: https://groups.google.com/g/django-developers/c/Hyqd1Rz6cFs
comment:7 by , 3 years ago
Good feature suggestion. The execution in captureOnCommitCallbacks would need extending too :)
comment:8 by , 3 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
follow-up: 11 comment:10 by , 3 years ago
Can this ticket be closed? Seems like the PR was accepted.
comment:11 by , 3 years ago
Replying to Shivan Sivakumaran:
Can this ticket be closed? Seems like the PR was accepted.
Not until the PR is merged, then it'll be closed as fixed
comment:12 by , 3 years ago
Cc: | added |
---|
comment:13 by , 3 years ago
Owner: | changed from | to
---|
comment:15 by , 2 years ago
Needs tests: | unset |
---|
comment:16 by , 2 years ago
Needs tests: | set |
---|---|
Patch needs improvement: | set |
comment:17 by , 2 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Patch needs improvement: | unset |
comment:18 by , 2 years ago
Needs tests: | set |
---|---|
Patch needs improvement: | set |
comment:19 by , 2 years ago
Needs tests: | unset |
---|---|
Patch needs improvement: | unset |
Triage Stage: | Accepted → Ready for checkin |
Sounds reasonable. Please take into account that the current behavior is documented.