Opened 9 years ago

Closed 4 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: master
Severity: Normal Keywords:
Cc: hv@…, farhan@…, 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 9 years ago.
commit_manually_exception_pass_thru.diff (2.0 KB) - added by justine <jtunney@…> 8 years ago.
commit_manually_exception_pass_thru2.diff (2.6 KB) - added by justine <jtunney@…> 8 years ago.
6623-4.diff (4.4 KB) - added by Claude Paroz 5 years ago.
Patch updated and with tests
6623-5.diff (5.0 KB) - added by k4ml 4 years ago.

Download all attachments as: .zip

Change History (27)

Changed 9 years ago by Thomas Güttler

comment:1 Changed 9 years ago by Bernd Schlapsi

I tested this patch today and it works fine for me

comment:2 Changed 9 years ago by Jeff Anderson

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 Changed 8 years ago by Farhan Ahmad

Cc: farhan@… added

comment:4 Changed 8 years ago by justine <jtunney@…>

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.

Changed 8 years ago by justine <jtunney@…>

comment:5 Changed 8 years ago by Malcolm Tredinnick

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).

Changed 8 years ago by justine <jtunney@…>

comment:6 Changed 8 years ago by James Bennett

milestone: 1.0post-1.0

comment:7 Changed 8 years ago by (none)

milestone: post-1.0

Milestone post-1.0 deleted

comment:8 Changed 7 years ago by daybreaker

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

comment:9 Changed 7 years ago by Jacob

Owner: changed from nobody to Jacob
Status: newassigned

comment:10 Changed 6 years ago by Julien Phalip

Type: Cleanup/optimization

comment:11 Changed 6 years ago by anonymous

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 Changed 6 years ago by Justine Tunney

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

comment:13 Changed 6 years ago by haavikko

Cc: haavikko added
Easy pickings: unset

comment:14 Changed 6 years ago by Julien Phalip

Needs tests: set

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

comment:15 Changed 5 years ago by umbrae@…

UI/UX: unset

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

Changed 5 years ago by Claude Paroz

Attachment: 6623-4.diff added

Patch updated and with tests

comment:16 Changed 5 years ago by Claude Paroz

Needs tests: unset

comment:17 Changed 4 years ago by Anssi Kääriäinen

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 Changed 4 years ago by Aymeric Augustin

#18449 was a duplicate.

comment:19 Changed 4 years ago by k4ml

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).

Changed 4 years ago by k4ml

Attachment: 6623-5.diff added

comment:20 Changed 4 years ago by k4ml

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 Changed 4 years ago by Aymeric Augustin

Owner: changed from Jacob to Aymeric Augustin

comment:22 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

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