Opened 7 years ago

Closed 2 years ago

#6623 closed Cleanup/optimization (fixed)

commit_manually decorator: Better error message (show exception)

Reported by: guettli Owned by: aaugustin
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 guettli 7 years ago.
commit_manually_exception_pass_thru.diff (2.0 KB) - added by justine <jtunney@…> 7 years ago.
commit_manually_exception_pass_thru2.diff (2.6 KB) - added by justine <jtunney@…> 7 years ago.
6623-4.diff (4.4 KB) - added by claudep 3 years ago.
Patch updated and with tests
6623-5.diff (5.0 KB) - added by k4ml 3 years ago.

Download all attachments as: .zip

Change History (27)

Changed 7 years ago by guettli

comment:1 Changed 7 years ago by Bernd Schlapsi

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

I tested this patch today and it works fine for me

comment:2 Changed 7 years ago by programmerq

  • Triage Stage changed from Unreviewed to Accepted

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 7 years ago by thebitguru

  • Cc farhan@… added

comment:4 Changed 7 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 7 years ago by justine <jtunney@…>

comment:5 Changed 7 years ago by mtredinnick

  • milestone set to 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 7 years ago by justine <jtunney@…>

comment:6 Changed 7 years ago by ubernostrum

  • milestone changed from 1.0 to post-1.0

comment:7 Changed 6 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:8 Changed 6 years ago by daybreaker

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

comment:9 Changed 6 years ago by jacob

  • Owner changed from nobody to jacob
  • Status changed from new to assigned

comment:10 Changed 4 years ago by julien

  • Type set to Cleanup/optimization

comment:11 Changed 4 years ago by anonymous

  • Severity set to 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 4 years ago by jart

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

comment:13 Changed 4 years ago by haavikko

  • Cc haavikko added
  • Easy pickings unset

comment:14 Changed 4 years ago by julien

  • Needs tests set

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

comment:15 Changed 3 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 3 years ago by claudep

Patch updated and with tests

comment:16 Changed 3 years ago by claudep

  • Needs tests unset

comment:17 Changed 3 years ago by akaariai

  • 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 3 years ago by aaugustin

#18449 was a duplicate.

comment:19 Changed 3 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 3 years ago by k4ml

comment:20 Changed 3 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 2 years ago by aaugustin

  • Owner changed from jacob to aaugustin

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

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

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