#20571: Using savepoints within transaction.atomic() can result in the entire
transaction being incorrectly and silently rolled back
---------------------------------+---------------------------------------
Reporter: lamby | Owner: aaugustin
Type: Bug | Status: assigned
Component: Documentation | Version: 1.6-alpha-1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+---------------------------------------
Comment (by aaugustin):
Anssi, I read your code, I read your arguments, and I understand what
you're trying to achieve, but I don't think that's the right way.
My work on transactions sets up a strict top-down control flow. The high-
level API (`atomic`) drives the low-level APIs (`commit/rollback`,
`savepoint_commit/rollback`). The low-level API never hooks back up inside
the high-level API. (Well, right now `commit/rollback` still reset the
dirty flag, but that's deprecated and going away.) Your proposal
introduces a reversed control flow, making it harder to reason about the
behavior, and I'm not comfortable with that.
My instinct initially told me not to include the `savepoint=False` option.
I eventually added it because you convinced me it was important and I
couldn't find any way to break it. However, this ticket shows that I
wasn't creative enough. Every additional complexity creates a risk of
unforeseen interactions, and I'm becoming paranoid, because any
transaction-related bug is a data loss bug.
----
In addition to the two proposals I made in comment 2, here's a third one:
add an advanced API to mark the innermost atomic block as needing or not
needing rollback.
This solution /probably/ works as expected, since the only purpose of
`needs_rollback` is to declare that an inner atomic block without a
savepoint exited with an exception and the innermost atomic block that has
a savepoint should exit with a rollback. By resetting `needs_rollback`,
you disable this behavior, which is the root cause of the bug described
here. As a matter of fact, that's already what I'm doing in
`TestCase._fixture_teardown`, except I'm forcing a rollback instead of
preventing one.
So we have two solutions: document `needs_rollback` as a public API, or
introduce a `getter` and a `setter` around `needs_rollback`. I'm in favor
of the second solution since it allows the API to live in
`django.db.transaction`. Proof of concept patch coming shortly.
--
Ticket URL: <https://code.djangoproject.com/ticket/20571#comment:9>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/063.10e2e69468486feb5d604bd4b1c54230%40djangoproject.com.
For more options, visit https://groups.google.com/groups/opt_out.
