Opened 2 years ago

Last modified 2 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 Daniel Hahler)

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

Change History (1)

comment:1 by Daniel Hahler, 2 years ago

Description: modified (diff)
Has patch: set
Note: See TracTickets for help on using tickets.
Back to Top