Opened 15 years ago

Closed 15 years ago

Last modified 13 years ago

#11900 closed (fixed)

commit_on_success: Handle Exception in commit()

Reported by: Thomas Güttler Owned by: Gabriel Hurley
Component: Database layer (models, ORM) Version: 1.0
Severity: Keywords:
Cc: Gabriel Hurley Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If you get an exception during the commit done by the commit_on_success decorator, you get a meaningless error message:

django.db.transaction.TransactionManagementError: Transaction managed block ended with pending COMMIT/ROLLBACK

With this patch you see the real exception.

Can someone please write a unittest for this?

Attachments (2)

transaction.py.patch (483 bytes ) - added by Thomas Güttler 15 years ago.
11900_patch_and_tests.diff (1.9 KB ) - added by Gabriel Hurley 15 years ago.
Updates patch to work w/ MultiDB and adds tests

Download all attachments as: .zip

Change History (11)

by Thomas Güttler, 15 years ago

Attachment: transaction.py.patch added

comment:1 by Thomas Güttler, 15 years ago

Version: 1.11.0

comment:2 by Russell Keith-Magee, 15 years ago

Has patch: set
milestone: 1.2
Needs tests: set
Triage Stage: UnreviewedAccepted

comment:3 by Gabriel Hurley, 15 years ago

Cc: Gabriel Hurley added
Owner: changed from nobody to Gabriel Hurley
Status: newassigned

The reason for the problem is clear enough: The error raised in the finally clause overrides the original error.

However, I tried for about 2 hours to reproduce the reported bug with no success.

guettli, can you some information about the code you're using that triggers the bug?

comment:4 by Thomas Güttler, 15 years ago

Hi Gabriel,

thank you for your question. This script results in

Traceback (most recent call last):
  File "djangotest_tbz/script.py", line 10, in <module>
    main()
  File "/home/djangotest_tbz_d/django/db/transaction.py", line 310, in _commit_on_success
    leave_transaction_management(using=db)
  File "/home/djangotest_tbz_d/django/db/transaction.py", line 87, in leave_transaction_management
    raise TransactionManagementError("Transaction managed block ended with pending COMMIT/ROLLBACK")
django.db.transaction.TransactionManagementError: Transaction managed block ended with pending COMMIT/ROLLBACK

I just tried it with latest trunk ([12761]). Postgres (psycopg2) evaluates Foreign-Key
Constraints lazy (deferrable). If you break this constraint, you get an exception
during commit. But you don't see the real problem.

from django.db import transaction, connection

@transaction.commit_on_success
def main():
    cursor=connection.cursor()
    cursor.execute('INSERT INTO auth_user_groups VALUES (DEFAULT, 10000000, 1000000);')
    transaction.set_dirty()

if __name__=='__main__':
    main()

by Gabriel Hurley, 15 years ago

Attachment: 11900_patch_and_tests.diff added

Updates patch to work w/ MultiDB and adds tests

comment:5 by Gabriel Hurley, 15 years ago

Needs tests: unset

This specific bug seems to be backend-specific (it seems to be completely impossible to trigger it using SQLite3). I did manage to duplicate it, test it, and patch it using a server running psycopg2. The general principle of the fix seems universally applicable, though.

The patch I added has both the requested test and an updated version of the original patch that works with the new MultiDB in 1.2. Hopefully it's good to go now.

comment:6 by Thomas Güttler, 15 years ago

Your patch is simple, contains a test and works for me. Thank you. Who can review and commit it?

comment:7 by Russell Keith-Magee, 15 years ago

Resolution: fixed
Status: assignedclosed

(In [12764]) Fixed #11900 -- Corrected an edge case of transaction handling in the commit_on_success decorator. Thanks to guettli for the report, and Gabriel Hurley for the initial test case.

comment:8 by Thomas Güttler, 14 years ago

Cc: hv@… removed

comment:9 by Jacob, 13 years ago

milestone: 1.2

Milestone 1.2 deleted

Note: See TracTickets for help on using tickets.
Back to Top