Opened 3 years ago
Last modified 3 years ago
#33331 closed Cleanup/optimization
Improve exception handling with `cursor.close()` after errors — at Version 1
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 (last modified by )
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.