Storm

Merge lp://staging/~jamesh/storm/bug-854787 into lp://staging/storm

bug-854787
Merge into trunk

Proposed by James Henstridge on 2011-09-22

Status:

Merged

Merged at revision:

411

Proposed branch:

lp://staging/~jamesh/storm/bug-854787

Merge into:

lp://staging/storm

Diff against target:

277 lines (+116/-22)

6 files modified

storm/database.py (+7/-3)
storm/databases/mysql.py (+3/-2)
storm/databases/postgres.py (+2/-2)
storm/django/backend/base.py (+9/-2)
tests/database.py (+34/-4)
tests/django/backend.py (+61/-9)

To merge this branch:

bzr merge lp://staging/~jamesh/storm/bug-854787

Critical

Fix Released

Link a bug report

Reviewer	Date Requested	Status
Stuart Bishop (community)	2011-09-22	Approve on 2011-09-30
Martin Albisetti (community)		Approve on 2011-09-29
Review via email: mp+76559@code.staging.launchpad.net

Description of the change

My previous patch to fix the detection of database disconnection errors in the Storm <-> Django bridge did not work correctly.

While it passed the DB access from the cursor through _check_disconnect(), the cursor it was acting on is one that the Django backend had wrapped to return Django's DatabaseError instead of the adapter's native exceptions. This meant that the is_disconnection_error() check would not even end up looking at the error.

This branch updates the disconnection checkers to handle the extra exception. It is not quite as neat as I'd like, so if anyone has suggestions that might improve it, I'm open to suggestions.

lp://staging/~jamesh/storm/bug-854787 updated on 2011-09-22

414. By James Henstridge on 2011-09-22: Import the Django exception from django.db instead of django.db.utils.

Revision history for this message

Stuart Bishop (stub) wrote on 2011-09-25:

You asked for suggestions too, so...

The disconnection errors don't change. Should the connection object have a method to register extra disconnection errors instead of them being passed in as arguments?

Instead of providing a list of exceptions and relying on the existing behavior of is_disconnection_error, should a callable be passed instead? This would provide more control to sniff the database exception when it isn't generic enough. Or is the goal here to insure the current is_disconnection_error logic is used because the PG backend already sniffs the errors strings?

How does this code handle and integrity violation (or other DatabaseError) btw? Given these seem to be thunked down from IntegrityError to DatabaseError it would be worth a test even if the test is demonstrating broken behavior and citing a bug in the Django bug tracker.

lp://staging/~jamesh/storm/bug-854787 updated on 2011-09-28

415. By James Henstridge on 2011-09-28: Add a test for the transaction registration code path after a database
disconnection (currently fails).
416. By James Henstridge on 2011-09-28: Register with the transaction manager when providing the connection to
Django.
417. By James Henstridge on 2011-09-28: Remove doubled register-transaction call.

Revision history for this message

James Henstridge (jamesh) wrote on 2011-09-28:

The object returned by DatabaseWrapper._cursor() ends up being:

1. StormCursorWrapper wrapping
2. CursorWrapper from django.db.backends.postgresql_psycopg2.base wrapping
3. a psycopg2 cursor

We're checking the exceptions at layer (1), and the exceptions are being modified at (2). The is_disconnection_error() code is different for each database backend, so I wanted to avoid duplicating the logic.

As for IntegrityError, there is no special handling in this branch because it isn't related to disconnection handling. The Django CursorWrapper classes do map those exceptions to their own IntegrityError though (I guess they think that error is important enough not to squash down to DatabaseError). I agree that it'd be better if Django didn't modify the error objects and used a strategy closer to what Storm does. I'll file a bug report about this, but I do want something that works with current Django releases.

I have also updated the branch to fix a related bug in the transaction registration code (and adds some extra tests that should have been in there from the start), which I believe fixes the last known problems related to disconnection handling.

Revision history for this message

James Henstridge (jamesh) wrote on 2011-09-28:

I've filed a bug report suggesting how Django could improve matters on their end:

https://code.djangoproject.com/ticket/16948

I'd still like to get something in place to handle current Django.

Revision history for this message

Martin Albisetti (beuno) on 2011-09-29:

review: Approve

Revision history for this message

Stuart Bishop (stub) on 2011-09-30:

review: Approve