[Django] #33331: Improve exception handling with `cursor.close()` after errors
Django
-------------------------------------+-------------------------------------
Reporter: Daniel | Owner: nobody
Hahler |
Type: | Status: new
Cleanup/optimization |
Component: Database | Version: 3.2
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
`cursor.close()` is explicitly after Exceptions with `cursor.execute()`,
and has a comment already that it might fail:
https://github.com/django/django/blob/322a1a037d4d2f18744c5d1a1efc2e84d4c5e94b/django/db/models/sql/compiler.py#L1210-L1215
{{{
try:
cursor.execute(sql, params)
except Exception:
# Might fail for server-side cursors (e.g. connection closed)
cursor.close()
}}}
The code there was changed to ignore any exception in
https://github.com/django/django/commit/6b6be692fcd102436c7abef1d7b3fa1d37ad4bdf
(Refs #16614), but then changed to the current code in
https://github.com/django/django/commit/d170c63351944fd91b2206d10f89e7ff75b53b76
(https://github.com/django/django/commit/d170c63351944fd91b2206d10f89e7ff75b53b76
#diff-f58de2deaccecd2d53199c5ca29e3e1050ec2adb80fb057cdfc0b4e6accdf14f).
The current approach however will create a chained exception always, and
especially will add it as context, and not as cause (see
https://blog.ram.rachum.com/post/621791438475296768/improving-python-
exception-chaining-with).
I suggest that exceptions in `cursor.close()` should get ignored as being
expected in a lot / most cased, and it being done only to ensure the
connection gets closed really, and should get `raised from` otherwise.
The main motivation here is that it makes it hard/confusing to see the
real error, e.g. when running tests or doing development, and the DB is
out of sync.
--
Ticket URL: <https://code.djangoproject.com/ticket/33331>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Old description:
> `cursor.close()` is explicitly after Exceptions with `cursor.execute()`,
> and has a comment already that it might fail:
> https://github.com/django/django/blob/322a1a037d4d2f18744c5d1a1efc2e84d4c5e94b/django/db/models/sql/compiler.py#L1210-L1215
>
> {{{
> try:
> cursor.execute(sql, params)
> except Exception:
> # Might fail for server-side cursors (e.g. connection closed)
> cursor.close()
> }}}
>
> The code there was changed to ignore any exception in
> https://github.com/django/django/commit/6b6be692fcd102436c7abef1d7b3fa1d37ad4bdf
> (Refs #16614), but then changed to the current code in
> https://github.com/django/django/commit/d170c63351944fd91b2206d10f89e7ff75b53b76
> (https://github.com/django/django/commit/d170c63351944fd91b2206d10f89e7ff75b53b76
> #diff-f58de2deaccecd2d53199c5ca29e3e1050ec2adb80fb057cdfc0b4e6accdf14f).
>
> The current approach however will create a chained exception always, and
> especially will add it as context, and not as cause (see
> https://blog.ram.rachum.com/post/621791438475296768/improving-python-
> exception-chaining-with).
>
> I suggest that exceptions in `cursor.close()` should get ignored as being
> expected in a lot / most cased, and it being done only to ensure the
> connection gets closed really, and should get `raised from` otherwise.
>
> The main motivation here is that it makes it hard/confusing to see the
> real error, e.g. when running tests or doing development, and the DB is
> out of sync.
New description:
`cursor.close()` is explicitly after Exceptions with `cursor.execute()`,
and has a comment already that it might fail:
https://github.com/django/django/blob/322a1a037d4d2f18744c5d1a1efc2e84d4c5e94b/django/db/models/sql/compiler.py#L1210-L1215
{{{
try:
cursor.execute(sql, params)
except Exception:
# Might fail for server-side cursors (e.g. connection closed)
cursor.close()
}}}
The code there was changed to ignore any exception in
https://github.com/django/django/commit/6b6be692fcd102436c7abef1d7b3fa1d37ad4bdf
(Refs #16614), but then changed to the current code in
https://github.com/django/django/commit/d170c63351944fd91b2206d10f89e7ff75b53b76
(https://github.com/django/django/commit/d170c63351944fd91b2206d10f89e7ff75b53b76
#diff-f58de2deaccecd2d53199c5ca29e3e1050ec2adb80fb057cdfc0b4e6accdf14f).
The current approach however will create a chained exception always, and
especially will add it as context, and not as cause (see
https://blog.ram.rachum.com/post/621791438475296768/improving-python-
exception-chaining-with).
I suggest that exceptions in `cursor.close()` should get ignored as being
expected in a lot / most cased, and it being done only to ensure the
connection gets closed really, and should get `raised from` otherwise.
The main motivation here is that it makes it hard/confusing to see the
real error, e.g. when running tests or doing development, and the DB is
out of sync.
PR: https://github.com/django/django/pull/15141
--
--
Ticket URL: <https://code.djangoproject.com/ticket/33331#comment:1>
Django
-------------------------------------+-------------------------------------
Cleanup/optimization |
Component: Database layer | Version: 3.2
(models, ORM) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => wontfix
Comment:
There was an extended discussion of the merits of the `raise...from`
syntax around #31177 ([https://groups.google.com/g/django-
developers/c/ibEOt3A9c2M/m/sOL7YV5LFwAJ Mailing list thread]).
The summary of that was that, in general, given that exceptions are
automatically chained, the different message **between** the chained
exceptions was not worth the noise.
The cited exceptions were using `raise...from` to remove noise from an
long-chain of exceptions, when you wished to pull-out the **real cause**
that may perhaps lie a frame frames deep, and using `from None` to
suppress chaining, when the underlying exception is not informative.
I'm going to close as wontfix on the basis of that discussion. If you
think this case is different you can message the DevelopersMailingList.
(If so, please give a concrete example of how the change is supposed to
help — I couldn't really see if from the description here — it seems like
a lot of effort to change the message between the tracebacks, but maybe
you're using the `__cause__` attribute in a way that merits it…? 🤔)
--
Ticket URL: <https://code.djangoproject.com/ticket/33331#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Daniel Hahler):
Sorry for not having read the mailing list thread yet, but only your
summary of it above.
I think the main issue with context vs. cause is an important detail. I
can see that it might not worth changing it all over the code base, but it
should be considered when fixing code.
The main issue I see here is that there will be an exception almost always
(at least with named cursors in PostgreSQL AFAICS), and therefore it
should not even be chained in this case: it really just hides the actual
error, just because it is trying to ensure a (never properly opened/used)
cursor is closed always.
I might follow up on the mailing list, but do not really feel like
subscribing there also etc, especially since the issue here is not about
context or cause, but only using `raise from` properly (according to
https://blog.ram.rachum.com/post/621791438475296768/improving-python-
exception-chaining-with) when not swallowing/hiding the expected
exception.
--
Ticket URL: <https://code.djangoproject.com/ticket/33331#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
Hi Daniel. Thanks for the reply.
It's OK to not read the thread :)
> ...not about context or cause, but only using raise from properly
(according to...
It's the force of ''properly'' here that's in doubt.
On investigation, the difference between the two syntaxes amounted to no
more (not much more) that the text used between the tracebacks — which are
chained either way, either ''implicitly'' or ''explicitly'', unless `from
None` is used. That difference in text, ''"During handling of the above
exception, another exception occurred"'' vs ''"The above exception was the
direct cause of the following exception"'', wasn't felt significant — not
compared to the more verbose syntax, and in the PR you have here, much
more verbose block in order to decide when to use it.
Thus, with the exceptions noted above, it was decided not to use the
`from` syntax. (We didn't agree with the blog post that not using it was
''improper''.)
So, if there's a concrete example, of ''"Look, you get this benefit"'',
then I'm really happy to look at that, and learn a trick, but pending
that, we have to go with the previous consensus. I hope that makes sense.
Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/33331#comment:4>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Daniel Hahler):
The main issue here is not about how to raise in case it should be raised,
but to not raise in the first place, if an error is expected.
It is meant to fix https://code.djangoproject.com/ticket/16614#comment:31
again, which regressed.
--
Ticket URL: <https://code.djangoproject.com/ticket/33331#comment:5>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: wontfix =>
--
Ticket URL: <https://code.djangoproject.com/ticket/33331#comment:6>
Django
-------------------------------------+-------------------------------------
Cleanup/optimization |
Component: Database layer | Version: 3.2
(models, ORM) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => needsinfo
Comment:
Hi Daniel.
I'm really sorry but I'm failing to see the point you're trying to make
here... There's clearly something I'm missing because you keep trying to
get it across to me, but I just don't get it. Can I **please** ask you to
take a step back and explain it to me from the top clearly so that I can
see the issue?
It looks to me like the code we have here when there's no exception in
`cursor.close()` is equivalent to this:
{{{
def will_raise(msg):
raise Exception(msg)
try:
will_raise("Outer")
except Exception:
raise
}}}
When I run that, I get this output:
{{{
% python3 exceptions.py
Traceback (most recent call last):
File "/Users/carlton/Desktop/exceptions.py", line 6, in <module>
will_raise("Outer")
File "/Users/carlton/Desktop/exceptions.py", line 2, in will_raise
raise Exception(msg)
Exception: Outer
}}}
When there **is** an exception in `cursor.close()` it looks like this:
{{{
def will_raise(msg):
raise Exception(msg)
try:
will_raise("Outer")
except Exception:
will_raise("Inner")
raise
}}}
Which gives this output:
{{{
% python3 exceptions.py
Traceback (most recent call last):
File "/Users/carlton/Desktop/exceptions.py", line 6, in <module>
will_raise("Outer")
File "/Users/carlton/Desktop/exceptions.py", line 2, in will_raise
raise Exception(msg)
Exception: Outer
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/Users/carlton/Desktop/exceptions.py", line 8, in <module>
will_raise("Inner")
File "/Users/carlton/Desktop/exceptions.py", line 2, in will_raise
raise Exception(msg)
Exception: Inner
}}}
Your suggestion seems to be to make it equivalent to:
{{{
def will_raise(msg):
raise Exception(msg)
try:
will_raise("Outer")
except Exception as o:
try:
will_raise("Inner")
except Exception as i:
raise i from o
raise
}}}
... which has the output:
{{{
% python3 exceptions.py
Traceback (most recent call last):
File "/Users/carlton/Desktop/exceptions.py", line 19, in <module>
will_raise("Outer")
File "/Users/carlton/Desktop/exceptions.py", line 15, in will_raise
raise Exception(msg)
Exception: Outer
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "/Users/carlton/Desktop/exceptions.py", line 24, in <module>
raise i from o
File "/Users/carlton/Desktop/exceptions.py", line 22, in <module>
will_raise("Inner")
File "/Users/carlton/Desktop/exceptions.py", line 15, in will_raise
raise Exception(msg)
Exception: Inner
}}}
Which is exactly the same bar the "direct cause" wording change, that was
agreed on the mailing list discussion to be not worth the verbosity of the
code change.
So what's it fixing? (Again: Sorry to not be seeing this, but I'm just not
getting the point you want to make.)
> It is meant to fix
https://code.djangoproject.com/ticket/16614#comment:31 again, which
regressed.
But it didn't regress. Both `Outer` and `Inner` are given in the
traceback. `Outer` is given first, plus the additional info that `Inner`
occurred whilst we were handling that.
This behaviour in Python 3 is what justifies the change in
https://github.com/django/django/commit/d170c63351944fd91b2206d10f89e7ff75b53b76
#diff-
f58de2deaccecd2d53199c5ca29e3e1050ec2adb80fb057cdfc0b4e6accdf14fR878-R879
— the original error **is** raised.
If you can put together an actual example of the problem you're hitting
coming up, that might make it easier. It's very hard to see when it's just
a couple of lines changed with no test-case or reproduce. (I appreciate
cursor errors can be hard to test...)
--
Ticket URL: <https://code.djangoproject.com/ticket/33331#comment:7>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Daniel Hahler):
The main thing this is meant to fix is not raising errors from
`cursor.close()` in the first place.
The difference between "caused by" and "then triggered also" is only
relevant for when it should not get swallowed (based on if it is expected
because there was an error already).
What you currently get during a test run with a missing migration:
{{{
Traceback (most recent call last):
File "…/Vcs/django/django/db/backends/utils.py", line 89, in _execute
return self.cursor.execute(sql, params)
psycopg2.errors.UndefinedColumn: column foo.bar does not exist
LINE 1: ...
^
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "…/Vcs/django/django/db/models/sql/compiler.py", line 1361, in
execute_sql
cursor.execute(sql, params)
…
File "…/Vcs/django/django/db/backends/utils.py", line 89, in _execute
return self.cursor.execute(sql, params)
django.db.utils.ProgrammingError: column foo.bar does not exist
LINE 1: ...
^
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "…/project/.venv/lib/python3.10/site-
packages/pytest_django/fixtures.py", line 122, in django_db_setup
db_cfg = setup_databases(
…
File "…/Vcs/django/django/db/models/sql/compiler.py", line 1364, in
execute_sql
cursor.close()
psycopg2.errors.InvalidCursorName: cursor
"_django_curs_140147764090688_sync_10" does not exist
}}}
`psycopg2.errors.InvalidCursorName` is expected to happen here, and
obfuscates/hides the real error (e.g. when it gets used for display in the
short test summary etc).
The (current) patch changes it to:
{{{
Traceback (most recent call last):
File "…/Vcs/django/django/db/backends/utils.py", line 89, in _execute
return self.cursor.execute(sql, params)
psycopg2.errors.UndefinedColumn: column foo.bar does not exist
LINE 1: ...
^
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "…/project/.venv/lib/python3.10/site-
packages/pytest_django/fixtures.py", line 122, in django_db_setup
db_cfg = setup_databases(
…
File "…/Vcs/django/django/db/backends/utils.py", line 89, in _execute
return self.cursor.execute(sql, params)
django.db.utils.ProgrammingError: column foo.bar does not exist
LINE 1: ...
^
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33331#comment:8>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
> The main thing … is not raising errors from cursor.close() in the first
place … e.g. when it gets used for display in the short test summary etc.
So the request is suppressing the chained nested exception — you don't
want the newer Python 3 behaviour in this case — which would be a new
feature, rather than a fixing a regression from #16614.
And the reason you want this if because your test runner (presumably
pytest) formats its output in a given way? (Is that it?)
--
Ticket URL: <https://code.djangoproject.com/ticket/33331#comment:9>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Daniel Hahler):
https://code.djangoproject.com/ticket/16614#comment:31 is about "Refs
#16614 -- Prevented database errors from being masked by cursor close.".
The fix there ignored all exceptions
(https://github.com/django/django/commit/6b6be692fcd102436c7abef1d7b3fa1d37ad4bdf).
The fix I've done for the regression is a bit smarter to still raise in
case of unexpected errors.
(This could also be removed in case you want a more concise patch, but
even when it is unlikely that it would swallow something unwanted, I've
meant to put attention to detail there - just in line with the difference
that we've previously discussed here)
--
Ticket URL: <https://code.djangoproject.com/ticket/33331#comment:10>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
Hi Daniel.
It wasn't really that 6b6be692fcd102436c7abef1d7b3fa1d37ad4bdf ignored all
exceptions. Rather it stopped the source exception being masked, with the
explicit intention that the new chained behaviour would be preferred when
it was available:
{{{
# Ignore clean up errors and raise the original error instead.
# Python 2 doesn't chain exceptions. Remove this error
# silencing when dropping Python 2 compatibility.
}}}
Given that it's clearly not a regression in
d170c63351944fd91b2206d10f89e7ff75b53b76 — that was the intended change
all along.
You're then asking for a change in behaviour to **remove** information
from the traceback. Maybe that's OK if there's a good reason, but
otherwise the presumption would be wontfix.
I'm not sure about the patch itself: it would be easier to consider in a
fresh PR, with a regression test.
--
Ticket URL: <https://code.djangoproject.com/ticket/33331#comment:11>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Daniel Hahler):
Ok, fair enough - it is only fixing a regression on Python 3 then (for
me), that was actually done as a workaround.
I suggest reading the original tickets description maybe then: my
intention was to not mask / "extend" the actual exception with more
information (the (IMHO incorrectly chained) error during cleanup, that is
expected to fail). It is similar to a `file.close()` when it is expected
that it wasn't created or its handler being invalid already.
Replying to
https://github.com/django/django/commit/6b6be692fcd102436c7abef1d7b3fa1d37ad4bdf#r67292630:
> The cleanup part of the chain adds noise to the actual source of the
error, so maybe it can be improved. But that’s an improvement over the
existing situation, not a regression.
Ok, I see. That's what I've meant to do: basically restoring the behavior
with Python 3 (when the workaround for Python 2 was in place), while being
a bit smarter to only ignore/swallow expected errors.
I was not aware that the intention with the Python 2 workaround was to
chain ("add useless noise") always with Python 3.
The question here really is also: if you would know that the connection is
invalid, and that `close()` would throw: would you call it in the first
place?
As for a new PR I'd rather update the existing one, but cannot re-open it
as mentioned there.
--
Ticket URL: <https://code.djangoproject.com/ticket/33331#comment:12>