Opened 3 years ago
Last modified 3 years ago
#33331 closed Cleanup/optimization
Improve exception handling with `cursor.close()` after errors — at Initial Version
| Reported by: | Daniel Hahler | Owned by: | nobody |
|---|---|---|---|
| Component: | Database layer (models, ORM) | Version: | 3.2 |
| Severity: | Normal | Keywords: | |
| Cc: | Triage Stage: | Unreviewed | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
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.