Opened 16 years ago

Closed 11 years ago

#6623 closed Cleanup/optimization (fixed)

commit_manually decorator: Better error message (show exception)

Reported by: Thomas Güttler Owned by: Aymeric Augustin
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: hv@…, farhan@…, Matti Haavikko Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hi,

if there is an unhandled exception in a 'commit_manually' method, it is very
hard to see the root of the problem, since you only see 'Transaction managed block ended with pending COMMIT/ROLLBACK'.

This patch creates a debug message, which gets passed to leave_transaction_management().

Attachments (5)

commit_manually_better_error_message.diff (2.0 KB ) - added by Thomas Güttler 16 years ago.
commit_manually_exception_pass_thru.diff (2.0 KB ) - added by justine <jtunney@…> 16 years ago.
commit_manually_exception_pass_thru2.diff (2.6 KB ) - added by justine <jtunney@…> 16 years ago.
6623-4.diff (4.4 KB ) - added by Claude Paroz 12 years ago.
Patch updated and with tests
6623-5.diff (5.0 KB ) - added by k4ml 12 years ago.

Download all attachments as: .zip

Change History (27)

by Thomas Güttler, 16 years ago

comment:1 by Bernd Schlapsi, 16 years ago

I tested this patch today and it works fine for me

comment:2 by Jeff Anderson, 16 years ago

Triage Stage: UnreviewedAccepted

This looks helpful, and the patch seems to work with current trunk. The last hunk was set to 'fuzzy' but it applied just fine.

comment:3 by Farhan Ahmad, 16 years ago

Cc: farhan@… added

comment:4 by justine <jtunney@…>, 16 years ago

I would very much like to see this bug fix committed, as I got burned by this today. I have a few comments on the patch though:

  • If the transaction was committed manually before the error was raised, this patch will eat up the exception
  • You cannot have except and finally in the same try block in python 2.4
  • I'm afraid I don't understand the significance of logging.error("ok")?
  • I don't think it's necessary to append the exception to the Commit error exception. The fact that the transaction wasn't committed is probably a side effect of whatever exception was raised.

I've attached a more up-to-date patch I think should work. Thanks everyone :-)

Also, should my error handler catch KeyboardInterrupt and SystemExit too? I'm not sure I understand that.

by justine <jtunney@…>, 16 years ago

comment:5 by Malcolm Tredinnick, 16 years ago

milestone: 1.0

I probably prefer the approach in the second patch, because the first exception is really going to be the real point here. The patch needs a bit of tweaking, mostly in light of #7241 and comment:4 of this ticket. Our current behaviour is hiding real problems and making our own code hard to debug (that transaction exception message appears in a few bugs where I suspect it's disguising the real issue).

by justine <jtunney@…>, 16 years ago

comment:6 by James Bennett, 16 years ago

milestone: 1.0post-1.0

comment:7 by (none), 15 years ago

milestone: post-1.0

Milestone post-1.0 deleted

comment:8 by daybreaker, 15 years ago

Is there anybody working on this, or any plan to adopt this?

comment:9 by Jacob, 14 years ago

Owner: changed from nobody to Jacob
Status: newassigned

comment:10 by Julien Phalip, 13 years ago

Type: Cleanup/optimization

comment:11 by anonymous, 13 years ago

Severity: Normal

The problem persists in Django 1.3... I spent 6 hours trying to DEBUG it. I used autocommit in the same code I found out real cause of problem. Hope it helps someone :)

comment:12 by Justine Tunney, 13 years ago

I submitted a trivial patch for this three years ago and I'm still getting emails? Seriously?

comment:13 by Matti Haavikko, 13 years ago

Cc: Matti Haavikko added
Easy pickings: unset

comment:14 by Julien Phalip, 13 years ago

Needs tests: set

This patch would need to also include tests to stand any chance of getting checked in.

comment:15 by umbrae@…, 12 years ago

UI/UX: unset

Just a "me, too" here. This is a serious issue for us on our high-volume production site at Readability.

by Claude Paroz, 12 years ago

Attachment: 6623-4.diff added

Patch updated and with tests

comment:16 by Claude Paroz, 12 years ago

Needs tests: unset

comment:17 by Anssi Kääriäinen, 12 years ago

Needs documentation: set

The patch needs a docs change. As currently documented, in commit_manually you must do a rollback manually, even if an exception is risen. Granted, the code does the rollback for you anyways currently.

This is a change (even if a small one) to documented behavior, so I think a heads-up post to django-developers should be done. I myself am +0 to this change.

comment:18 by Aymeric Augustin, 12 years ago

#18449 was a duplicate.

comment:19 by k4ml, 12 years ago

I think the docs only require you to do rollback manually if you managed to catch the exception. In this case the exception goes uncaught from your function, thus we're having this issue since we don't know that we need to do a rollback. So I have added a note to the docs on django doing rollback in case there's an exception goes uncaught the function under commit_manually.

Also the previous patch does not apply cleanly anymore to django master (github.com/django/django).

by k4ml, 12 years ago

Attachment: 6623-5.diff added

comment:20 by k4ml, 12 years ago

Another note, the rollback when exception occurred not introduced by this patch, it's already in the committed code since Django-1.3, this patch only raise the exception instead of TransactionManagementError.

comment:21 by Aymeric Augustin, 11 years ago

Owner: changed from Jacob to Aymeric Augustin

comment:22 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 14cddf51c5f001bb426ce7f7a83fdc52c8d8aee9:

Merged branch 'database-level-autocommit'.

Fixed #2227: atomic supports nesting.
Fixed #6623: commit_manually is deprecated and atomic doesn't suffer from this defect.
Fixed #8320: the problem wasn't identified, but the legacy transaction management is deprecated.
Fixed #10744: the problem wasn't identified, but the legacy transaction management is deprecated.
Fixed #10813: since autocommit is enabled, it isn't necessary to rollback after errors any more.
Fixed #13742: savepoints are now implemented for SQLite.
Fixed #13870: transaction management in long running processes isn't a problem any more, and it's documented.
Fixed #14970: while it digresses on transaction management, this ticket essentially asks for autocommit on PostgreSQL.
Fixed #15694: atomic supports nesting.
Fixed #17887: autocommit makes it impossible for a connection to stay "idle of transaction".

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