Re: [Django] #36318: Bad stack trace during rollback after bulk create

[Django]

#36318: Bad stack trace during rollback after bulk create
-------------------------------------+-------------------------------------
Reporter: Gordon Wrigley | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | 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 Adam Johnson):

* component: Uncategorized => Database layer (models, ORM)
* stage: Unreviewed => Accepted
* type: Uncategorized => Bug

Comment:

Thank you for the clear bug report. It would be good to try with `main` to
check if the behaviour persists, but from a quick look at
`Atomic.__exit__`, it seems likely that it does.
--
Ticket URL: <https://code.djangoproject.com/ticket/36318#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36318: Bad stack trace during rollback after bulk create
-------------------------------------+-------------------------------------
Reporter: Gordon Wrigley | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | 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 Simon Charette):

I'll point out that we document that weird things can happen when you
catch and handle exceptions inside an atomic block

Per
[https://docs.djangoproject.com/en/5.2/topics/db/transactions/#django.db.transaction.atomic
the docs]

> **Avoid catching exceptions inside atomic!
>
> When exiting an `atomic` block, Django looks at whether it’s exited
normally or with an exception to determine whether to commit or roll back.
If you catch and handle exceptions inside an atomic block, you may hide
from Django the fact that a problem has happened. **This can result in
unexpected behavior.**

I'm pretty confident we'll be able to avoid surfacing the previous
exception message as the source of the `TransactionManagementError` with

{{{#!diff
diff --git a/django/db/transaction.py b/django/db/transaction.py
index 0c2eee8e73..f56ecfc085 100644
--- a/django/db/transaction.py
+++ b/django/db/transaction.py
@@ -195,6 +195,7 @@ def __enter__(self):
# Reset state when entering an outermost atomic block.
connection.commit_on_exit = True
connection.needs_rollback = False
+ connection.rollback_exc = None
if not connection.get_autocommit():
# Pretend we're already in an atomic block to bypass the
code
# that disables autocommit to enter a transaction, and
make a
}}}

as this is a bug in #23353 (3b4a5b9f97f113ca5151cff744019e39a1ed7475) but
I'm afraid we won't able to have the `TransactionManagementError` provide
details about the `ValidationError` or any exception raised in an
exception handled inside an atomic block for that matter as it it doesn't
make it do `Atomic.__exit__` well it can't be stored anywhere.
--
Ticket URL: <https://code.djangoproject.com/ticket/36318#comment:4>

[Django]

#36318: Bad stack trace during rollback after bulk create
-------------------------------------+-------------------------------------
Reporter: Gordon Wrigley | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | 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 Gordon Wrigley):

I put prints through `Atomic.__exit__` and I see the `ValidationError` in
there. One time when it passes through `exc_type` is the
`ValidationError`, `needs_rollback` is False and the structure of the
logic leads to it setting `needs_rollback` to True
--
Ticket URL: <https://code.djangoproject.com/ticket/36318#comment:5>

[Django]

#36318: Bad stack trace during rollback after bulk create
-------------------------------------+-------------------------------------
Reporter: Gordon Wrigley | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | 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 Simon Charette):

I think the following might address the issue (for the usage of
`atomic(savepoint=False)` that `bulk_create` makes)

{{{#!diff
diff --git a/django/db/transaction.py b/django/db/transaction.py

index 0c2eee8e73..31363e686b 100644

--- a/django/db/transaction.py
+++ b/django/db/transaction.py
@@ -195,6 +195,7 @@ def __enter__(self):
# Reset state when entering an outermost atomic block.
connection.commit_on_exit = True
connection.needs_rollback = False
+ connection.rollback_exc = None
if not connection.get_autocommit():
# Pretend we're already in an atomic block to bypass the
code
# that disables autocommit to enter a transaction, and
make a

@@ -278,6 +279,9 @@ def __exit__(self, exc_type, exc_value, traceback):
# otherwise.
if sid is None:
connection.needs_rollback = True
+ # Avoid shadowing an already assigned rollback
exc.
+ if connection.rollback_exc is None:
+ connection.rollback_exc = exc_value
else:
try:
connection.savepoint_rollback(sid)
diff --git a/tests/transactions/tests.py b/tests/transactions/tests.py
index 9fe8c58593..329f2e74c2 100644
--- a/tests/transactions/tests.py
+++ b/tests/transactions/tests.py
@@ -504,6 +504,27 @@ def
test_mark_for_rollback_on_error_in_autocommit(self):
# … and further queries work nicely.
Reporter.objects.create()

+ def test_transaction_management_cause_leak(self):
+ with self.assertRaises(Exception), transaction.atomic():
+ raise Exception("First failure")
+
+ with (
+ self.assertRaises(transaction.TransactionManagementError) as
ctx,
+ transaction.atomic(),
+ ):
+ try:
+ with transaction.atomic(savepoint=False):
+ raise Exception("Second failure")
+ except Exception:
+ pass
+ try:
+ with transaction.atomic(savepoint=False):
+ raise Exception("Third failure")
+ except Exception:
+ pass
+ Reporter.objects.count()
+ self.assertEqual(str(ctx.exception.__cause__), "Second failure")
+

class NonAutocommitTests(TransactionTestCase):
available_apps = []
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36318#comment:6>

[Django]

#36318: Bad stack trace during rollback after bulk create
-------------------------------------+-------------------------------------
Reporter: Gordon Wrigley | Owner:

| JaeHyuckSa
Type: Bug | Status: assigned

Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | 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 JaeHyuckSa):

* owner: (none) => JaeHyuckSa
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36318#comment:7>

[Django]

#36318: Bad stack trace during rollback after bulk create
-------------------------------------+-------------------------------------
Reporter: Gordon Wrigley | Owner:
| JaeHyuckSa
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | 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 JaeHyuckSa):

Thank you, Gordon and Simon, for the detailed explanation. I'll go ahead
and prepare a PR with the suggested test and fix.
--
Ticket URL: <https://code.djangoproject.com/ticket/36318#comment:8>

[Django]

#36318: Bad stack trace during rollback after bulk create
-------------------------------------+-------------------------------------
Reporter: Gordon Wrigley | Owner:
| JaeHyuckSa
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by JaeHyuckSa):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36318#comment:9>

[Django]

#36318: Bad stack trace during rollback after bulk create
-------------------------------------+-------------------------------------
Reporter: Gordon Wrigley | Owner:
| JaeHyuckSa
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 1

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by kevingill1966):

* needs_docs: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36318#comment:10>

[Django]

#36318: Bad stack trace during rollback after bulk create
-------------------------------------+-------------------------------------
Reporter: Gordon Wrigley | Owner:
| JaeHyuckSa
Type: Bug | Status: assigned
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by JaeHyuckSa):

* needs_docs: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/36318#comment:11>