#33331: Improve exception handling with `cursor.close()` after errors
-------------------------------------+-------------------------------------
Reporter: Daniel Hahler | Owner: nobody
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
-------------------------------------+-------------------------------------
Changes (by Daniel Hahler):
* 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 <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 view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/065.525d17d665d7f61a42792fa11f9538c6%40djangoproject.com.
