commit_manually decorator: Better error message (show exception)

[Thomas Güttler]

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

[Bernd Schlapsi]

I tested this patch today and it works fine for me

[Jeff Anderson]

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

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

[Malcolm Tredinnick]

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

[(none)]

Milestone post-1.0 deleted

[daybreaker]

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

[anonymous]

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

[Justine Tunney]

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

[Julien Phalip]

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

[umbrae@…]

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

[Claude Paroz]

Patch updated and with tests

[Anssi Kääriäinen]

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.

[Aymeric Augustin]

#18449 was a duplicate.

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

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

[Aymeric Augustin <aymeric.augustin@…>]

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