Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 years ago

#7241 closed (fixed)

Transaction management does not catch all exceptions

Reported by: jim <jim-django@…> Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Expectation:
When a view is decorated with commit_on_success, any exception raised in that view should cause a rollback of the current transaction. If no exception occurs, the transaction should be committed.

Experience:
There are many exceptions that are not caught by commit_on_success. When one of these is raised, neither a rollback nor a commit is done. This causes a TransactionManagementError in leave_transaction_management().

The only exceptions caught are subclasses of Python's built in Exception class. Since Python allows any object to be raised as an exception, this leaves an unprotected path in the code.

For example (untested, just an illustration):

@commit_on_success
def my_view(request):
   raise "simple debug exception"

The object being raised will not match commit_on_success's handler, and the error condition is triggered.

Note: Yes, string exceptions are deprecated. But they are still possible, and must be guarded against.

Simple patch attached.

Attachments (1)

catchmore.patch (437 bytes) - added by jim <jim-django@…> 6 years ago.
simple patch

Download all attachments as: .zip

Change History (6)

Changed 6 years ago by jim <jim-django@…>

simple patch

comment:1 Changed 6 years ago by Bernd Schlapsi

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I think #6623 is related to this issue

comment:2 Changed 6 years ago by jim <jim-django@…>

Notice that this is not the classic misguided "naked except", as it re-raises immediately after the rollback.

comment:3 Changed 6 years ago by ericholscher

  • milestone set to 1.0
  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 6 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

(In [8419]) Fixed #7241 -- More robust exception catching in the transaction management
code. As pointed out in the ticket, Python still lets you raise all sorts of
odd things as exceptions (e.g. strings), so even though they're bad form, we
should still handle them. We do that cleanly now. Thanks to jim-django@…
for the patch.

comment:5 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.