Opened 3 weeks ago
Last modified 2 weeks ago
#37369 assigned New feature
on_commit should raise an error when no transaction is open
| Reported by: | Charlie Denton | Owned by: | Vishy |
|---|---|---|---|
| Component: | Database layer (models, ORM) | Version: | 6.1 |
| Severity: | Normal | Keywords: | |
| Cc: | Charlie Denton, 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
When calling on_commit outside of a transaction, Django will execute the callback immediately. This is a documented feature. See https://docs.djangoproject.com/en/6.1/topics/db/transactions/#performing-actions-after-commit
This leads to misleading code which looks as though it will defer work when it will not. For example, it masks issues where on_commit is called with a different using= argument to the transaction which is currently open. (Either the transaction was opened on the wrong DB, or the callback is waiting on the wrong database.)
Instead, it would make sense to adopt the behaviour of Django Subatomic's run_after_commit, which raises an error when on_commit is called outside of a transaction. After all, there will be no COMMIT when there is no open transaction.
This idea was suggested when on_commit was introduced, but was seemingly not addressed. See:
- https://github.com/django/django/pull/4593#issuecomment-98496218
- https://github.com/django/django/pull/4593#issuecomment-98762940
This has been discussed at Django on the Med 2026. The approach that was suggested to me was:
Step 1:
- Change
on_committo raise an error when it is called without an open transaction. - Add a temporary setting to opt out of the new behaviour (and emit a warning when
on_commitis called with no transaction).
Step 2:
- Remove the opt-out setting.
To prevent a situation where code passes in tests but fails in production, we should explicitly ignore the transaction created by the test suite when checking if a transaction is open. Django Subatomic does this check using a utility function: in_transaction. I believe that function would be a useful addition to Django's API. See https://kraken-tech.github.io/django-subatomic/v2.0.0/reference/django_subatomic/db/#django_subatomic.db.in_transaction and https://github.com/django/new-features/issues/147.
The in_transaction API will enable developers who want to run the callback immediately if there is no transaction:
if in_transaction(): on_commit(my_callback) else: my_callback()
Change History (7)
comment:1 by , 3 weeks ago
| Triage Stage: | Unreviewed → Accepted |
|---|
comment:2 by , 3 weeks ago
| Owner: | set to |
|---|---|
| Status: | new → assigned |
comment:4 by , 3 weeks ago
We considered adding a new flag to on_commit, but decided that was a bad API. I don't think we considered a separate function.
I think the explicit in_transaction check is better, despite the verbosity - it makes it very clear what is going on. A project can also define defer_if_in_transaction locally if they use that pattern a lot and find the verbosity a problem.
I'm open to being persuaded otherwise though.
comment:5 by , 3 weeks ago
A project can also define
defer_if_in_transactionlocally if they use that pattern a lot and find the verbosity a problem.
I'm open to being persuaded otherwise though.
It feels like something that would be worthy of at least a forum post to gather more feedback.
From my personal experience it's pretty common for external side effect causing calls (e.g. background tasks scheduling) to use on_commit in its current form in case they are called within a transaction. Not providing a blessed API for this pattern assumes that application that made use of it over the past decade will eventually be refactored to make sure that every function they provide explicit define whether they expect to run in a transaction or not which seems unrealistic particularly for third party party applications.
comment:6 by , 3 weeks ago
The core issue—that on_commit executing immediately can mask bugs—is very valid. However, I agree with Simon regarding the third-party applications and think we need a consensus on naming and how this impacts the broader ecosystem.
Many third-party apps don't control the user's execution context but must support both transactional and non-transactional calls. Forcing them all to implement
custom if in_transaction(): ... wrappers creates unnecessary boilerplate. Introducing a blessed transaction.on_commit_or_execute() (naming TBD) gives the ecosystem a safe, drop-in replacement.
comment:7 by , 2 weeks ago
Introducing a new after_commit_else_immediate() (or whatever) API seems reasonable to me, if the dangers are clearly documented. It is significantly better than introducing an actually_don't_defer= option to the existing API, and is at least clear to callers that their code is expected to handle being called both in and out of a transaction. (I don't think it's a good idea to write code like that, but I understand that I'm stricter on that issue than many others might choose to be.)
Have we considered adding a new
transactionAPI replacement that implements the current behaviour instead of taking it away entirely?I understand the rationale for defaulting to having
on_commitraise when called outside of a transaction (mainly due its name) but I believe there is nonetheless merit in having a public API that allows the conditional deferring of an operation in code paths that may or may not be called within a transaction. It feels like a common enough pattern not to warrant making an explicitin_transactioncheck this is needed?If we introduced a
defer_if_in_transaction(better name very welcome) public API as a replacement we wouldn't require a setting as the deprecation warning could simply point at using it instead ofon_commit(when the latter is called outside of a transaction) before we finally change the default behaviour ofon_commit.