[Django] #20571: Using savepoints within transaction.atomic() can result in the entire transaction being incorrectly and silently rolled back
Django
transaction being incorrectly and silently rolled back
----------------------------------------------+--------------------
Reporter: lamby | Owner: nobody
Type: Uncategorized | Status: new
Component: Database layer (models, ORM) | Version: 1.5
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
Using savepoints within transaction.atomic() can result in the entire
transaction being incorrectly and silently rolled back. Here is a slightly
contrived example:
{{{#!python
from django.db import transaction, DatabaseError
with transaction.atomic():
user = User.objects.all()[0]
user.last_login = datetime.datetime.utcnow() # any change to
demonstrate problem
user.save()
sid = transaction.savepoint()
try:
# Will always fail
User.objects.create(pk=user.pk)
transaction.savepoint_commit(sid)
except DatabaseError:
transaction.savepoint_rollback(sid)
# outside of atomic() context - user.last_login change has not been
committed!
}}}
What happens is the .create() call fails and marks the outermost atomic
block as requiring a rollback, so even though we call savepoint_rollback
(which does exactly right thing), once we leave the outer
transaction.atomic(), the entire transaction is rolled back. In the
example above, this means that the change to last_login is not persisted
in the database. Rather insiduously, it is done entirely silently. :)
(The outer transaction.atomic() seems a little contrived but note that it
is equivalent to the wrapper added by ATOMIC_REQUESTS.)
I'm not entirely sure what the solution is; the
transaction.atomic(savepoint=False) call within ModelBase.save_base simply
does not (and cannot) know that some other code will manually handle the
savepoint rollback and thus no choice but to mark the entire transaction
as borked. The only way it could know is if .create and .save took yet
another kwarg, but this seems bizarre and would not be at all obvious.
Maybe manual savepoints should be become a private API after all
(contradicting the current note in the documentation). Alternatively, it
could be clarified that "manual" savepoint management they should not be
used in conjunction with atomic() blocks (and thus ATOMIC_REQUESTS). The
behaviour above is certainly not obvious at all.
(Hm, update_or_create uses savepoints manually, I hope that isn't breaking
installations?)
--
Ticket URL: <https://code.djangoproject.com/ticket/20571>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
transaction being incorrectly and silently rolled back
Reporter: lamby | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version:
(models, ORM) | 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
Changes (by akaariai):
* severity: Normal => Release blocker
* needs_better_patch: => 0
* needs_tests: => 0
* version: 1.5 => 1.6-alpha-1
* needs_docs: => 0
* type: Uncategorized => Bug
* stage: Unreviewed => Accepted
Comment:
Ill mark this as release blocker, seems like some solution is going to be
needed... I haven't tested anything, but I feel confident enough in the
report to just mark this accepted, too.
I wonder if savepoint_commit() and savepoint_rollback() could just mark
the transaction block as "correct" again. Wouldn't this solve the problem?
--
Ticket URL: <https://code.djangoproject.com/ticket/20571#comment:1>
Django
transaction being incorrectly and silently rolled back
Reporter: lamby | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version:
Component: Documentation | 1.6-alpha-1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* type: Bug => Cleanup/optimization
* component: Database layer (models, ORM) => Documentation
Comment:
One solution is to remove the `savepoint=False` option. But it was
included for two good reasons:
- it improves performance,
- it avoids breaking all assertNumQueries with extra SAVEPOINT / RELEASE
SAVEPOINT queries.
Another solution is to discourage using the low level API. Technically,
the inner block in the example above is strictly equivalent to:
{{{
with transaction.atomic():
User.objects.create(pk=user.pk)
}}}
I'm in favor of the second solution because I still have to see a use case
that isn't covered by `transaction.atomic()`. There aren't many patterns
for using savepoints in an application.
--
Ticket URL: <https://code.djangoproject.com/ticket/20571#comment:2>
Django
transaction being incorrectly and silently rolled back
Reporter: lamby | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version:
Component: Documentation | 1.6-alpha-1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by akaariai):
This is the idea I was considering:
https://github.com/akaariai/django/compare/manual_savepoint_atomic - seems
to work OK.
--
Ticket URL: <https://code.djangoproject.com/ticket/20571#comment:3>
Django
transaction being incorrectly and silently rolled back
Reporter: lamby | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version:
Component: Documentation | 1.6-alpha-1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by lamby):
Replying to [comment:2 aaugustin]:
> Technically, the inner block in the example above is strictly equivalent
to:
>
> {{{
> with transaction.atomic():
> User.objects.create(pk=user.pk)
> }}}
Yes, although one still needs to catch the DatabaseError to be literally
identical. :)
> I'm in favor of the second solution because I still have to see a use
case that isn't covered by `transaction.atomic()`. There aren't many
patterns for using savepoints in an application.
Just for clarity: I assume here you mean there aren't many patterns for
using savepoints *that are not also covered by using transaction.atomic*.
> I wonder if savepoint_commit() and savepoint_rollback() could just mark
the transaction block as "correct" again. Wouldn't this solve the problem?
No. That's too blunt as there is no way to know we are rolling back to a
savepoint that results in a clean block. I'm not explaining that very
well, so here's an untested example:
{{{
with transaction.atomic():
s1 = transaction.savepoint()
try:
User.objects.create(pk=user.pk)
except DatabaseError:
# block now correctly marked as requiring rollback
s2 = transaction.savepoint()
# [..]
transaction.savepoint_rollback(s2)
# Oops. Assuming your solution, the block would now be incorrectly
# marked as *not* requiring rollback even though it is required
# due to the User.objects.create failure. We would only be able to
# mark the block as clean if we rolled back to s1, but we have no
# way of knowing that.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20571#comment:4>
Django
transaction being incorrectly and silently rolled back
Reporter: lamby | Owner: nobody
Type: Bug | Status: new
Component: Documentation | Version: 1.6-alpha-1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by lamby):
* cc: lamby (added)
* type: Cleanup/optimization => Bug
Comment:
[Reverting to "bug"; seemed like an accidental change considering the
severity is "release blocker".]
--
Ticket URL: <https://code.djangoproject.com/ticket/20571#comment:5>
Django
transaction being incorrectly and silently rolled back
Reporter: lamby | Owner: nobody
Type: Bug | Status: new
Component: Documentation | Version: 1.6-alpha-1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by akaariai):
I see. But isn't this a problem in current Django code, too:
{{{
with transaction.atomic():
try:
User.objects.create(pk=user.pk) # exception marks connection
needing rollback
except Exception:
with transaction.atomic():
# do something that raises exception
# exception marks needs_rollback = False
# Whoops, the outer block is marked clean!
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20571#comment:6>
Django
transaction being incorrectly and silently rolled back
Reporter: lamby | Owner: nobody
Type: Bug | Status: new
Component: Documentation | Version: 1.6-alpha-1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by akaariai):
I was wrong in the above example, the second transaction.atomic() doesn't
create any savepoints as the whole transaction needs to be rolled back
anyways. Thus the needs_rollback flag isn't cleared.
I updated the
https://github.com/akaariai/django/compare/manual_savepoint_atomic patch.
Now you can't create savepoints in failed blocks, so s2 in the example of
comment:4 would fail. This seems good enough, there really isn't any point
in creating a savepoint which is going to be rolled back anyways.
As for do we need the ability to use manual savepoints? If possible, yes.
Some things become nasty to code if your only option is using exceptions.
Say, you call a method, and if that method returns false you will need to
rollback the current savepoint. Options:
{{{
def myf():
sp = savepoint()
if somemethod():
savepoint_commit()
else:
savepoint_rollback()
def myf():
try:
with atomic():
if somemethod():
return
else:
raise FakeException # This will now rollback the
savepoint.
except FakeException:
return
}}}
Another situation where atomic() isn't easy is if you have different paths
for enter and exit. Then naturally any with statement can't be used. An
example is TransactionMiddleware.
--
Ticket URL: <https://code.djangoproject.com/ticket/20571#comment:7>
Django
transaction being incorrectly and silently rolled back
Reporter: lamby | Owner: anonymous
Type: Uncategorized | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: 1 | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 1
-------------------------------+--------------------------------------
Changes (by ogpcludi <sample@…>):
* status: new => assigned
* severity: Release blocker => Normal
* needs_better_patch: 0 => 1
* component: Documentation => contrib.admin
* needs_tests: 0 => 1
* owner: nobody => anonymous
* version: 1.6-alpha-1 => master
* easy: 0 => 1
* keywords: => 1
* needs_docs: 0 => 1
* has_patch: 0 => 1
* ui_ux: 0 => 1
* type: Bug => Uncategorized
* stage: Accepted => Unreviewed
Comment:
1
--
Ticket URL: <https://code.djangoproject.com/ticket/20571#comment:8>
Django
transaction being incorrectly and silently rolled back
Reporter: lamby | Owner: nobody
Type: Uncategorized | Status: new
Severity: Normal | Resolution:
Keywords: 1 | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 1
-------------------------------+--------------------------------------
Changes (by ogpcludi <sample@…>):
* severity: Release blocker => Normal
* needs_better_patch: 0 => 1
* component: Documentation => contrib.admin
* needs_tests: 0 => 1
* version: 1.6-alpha-1 => master
* easy: 0 => 1
* keywords: => 1
* needs_docs: 0 => 1
* has_patch: 0 => 1
* ui_ux: 0 => 1
* type: Bug => Uncategorized
* stage: Accepted => Unreviewed
Comment:
1
--
Ticket URL: <https://code.djangoproject.com/ticket/20571#comment:9>
Django
transaction being incorrectly and silently rolled back
Reporter: lamby | Owner: nobody
Type: Uncategorized | Status: closed
Component: contrib.admin | Version: master
Severity: Normal | Resolution: fixed
Keywords: 1 | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 1
-------------------------------+--------------------------------------
Changes (by ogpcludi <sample@…>):
* status: new => closed
* severity: Release blocker => Normal
* needs_better_patch: 0 => 1
* component: Documentation => contrib.admin
* needs_tests: 0 => 1
* version: 1.6-alpha-1 => master
* easy: 0 => 1
* type: Bug => Uncategorized
* keywords: => 1
* needs_docs: 0 => 1
* has_patch: 0 => 1
* ui_ux: 0 => 1
* resolution: => fixed
* stage: Accepted => Unreviewed
Comment:
1
--
Ticket URL: <https://code.djangoproject.com/ticket/20571#comment:10>
Django
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
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* owner: nobody => aaugustin
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/20571#comment:8>
Django
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
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
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
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
I've written the patch and I'm now 100% sure it's the way to go. I'm
committing it.
--
Ticket URL: <https://code.djangoproject.com/ticket/20571#comment:10>
Django
transaction being incorrectly and silently rolled back
Reporter: lamby | Owner: aaugustin
Type: Bug | Status: assigned
(models, ORM) | 1.6-alpha-1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* component: Documentation => Database layer (models, ORM)
--
Ticket URL: <https://code.djangoproject.com/ticket/20571#comment:11>
Django
transaction being incorrectly and silently rolled back
Reporter: lamby | Owner: aaugustin
Component: Database layer | Version:
(models, ORM) | 1.6-alpha-1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Aymeric Augustin <aymeric.augustin@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"c1284c3d3c6131a9d0ded9601ae0feb9a2e81a65"]:
{{{
#!CommitTicketReference repository=""
revision="c1284c3d3c6131a9d0ded9601ae0feb9a2e81a65"
Fixed #20571 -- Added an API to control connection.needs_rollback.
This is useful:
- to force a rollback on the exit of an atomic block without having to
raise and catch an exception;
- to prevent a rollback after handling an exception manually.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20571#comment:12>
Django
transaction being incorrectly and silently rolled back
Reporter: lamby | Owner: aaugustin
Type: Bug | Status: closed
Component: Database layer | Version:
(models, ORM) | 1.6-alpha-1
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by akaariai):
I think Django should prevent running *anything else* than rollback or
savepoint rollback once inside a transaction that is marked as
needs_rollback. This is what PostgreSQL does, and with good reason -
letting users continue a transaction that will be rolled back anyways will
lead to errors. Addition of needs_rollback API lets users continue after
errors when needed, but do so only explicitly.
BTW I think I proposed adding @in_transaction decorator, not
atomic(savepoint=False). This would have been no-op if a transaction was
going on, otherwise it would have created a transaction. This is subtly
different from @atomic(savepoint=False) which marks the outer block for
needs_rollback on errors. @in_transaction is what is needed by
model.save() for example (and yes, using savepoints unconditionally in
that case is too expensive).
In any case, this ticket is already solved so time to move on.
--
Ticket URL: <https://code.djangoproject.com/ticket/20571#comment:13>