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

[James Henstridge]

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:~jamesh/storm/bug-854787]

414. By James Henstridge on 2011-09-22

Import the Django exception from django.db instead of django.db.utils.

[Stuart Bishop (stub)]

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:~jamesh/storm/bug-854787]

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.

[James Henstridge (jamesh)]

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.

[James Henstridge (jamesh)]

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.

[Martin Albisetti (beuno)]

review: Approve

[Stuart Bishop (stub)]

review: Approve

=== modified file 'storm/database.py'
--- storm/database.py	2011-09-12 10:56:26 +0000
+++ storm/database.py	2011-09-28 06:23:18 +0000
@@ -353,7 +353,7 @@
             else:
                 self._state = STATE_CONNECTED
 
-    def is_disconnection_error(self, exc):
+    def is_disconnection_error(self, exc, extra_disconnection_errors=()):
         """Check whether an exception represents a database disconnection.
 
         This should be overridden by backends to detect whichever
@@ -363,10 +363,14 @@
 
     def _check_disconnect(self, function, *args, **kwargs):
         """Run the given function, checking for database disconnections."""
+        # Allow the caller to specify additional exception types that
+        # should be treated as possible disconnection errors.
+        extra_disconnection_errors = kwargs.pop(
+            'extra_disconnection_errors', ())
         try:
             return function(*args, **kwargs)
-        except Error, exc:
-            if self.is_disconnection_error(exc):
+        except Exception, exc:
+            if self.is_disconnection_error(exc, extra_disconnection_errors):
                 self._state = STATE_DISCONNECTED
                 self._raw_connection = None
                 raise DisconnectionError(str(exc))
 
=== modified file 'storm/databases/mysql.py'
--- storm/databases/mysql.py	2009-09-21 17:03:08 +0000
+++ storm/databases/mysql.py	2011-09-28 06:23:18 +0000
@@ -114,9 +114,10 @@
             else:
                 yield param
 
-    def is_disconnection_error(self, exc):
+    def is_disconnection_error(self, exc, extra_disconnection_errors=()):
         # http://dev.mysql.com/doc/refman/5.0/en/gone-away.html
-        return (isinstance(exc, OperationalError) and
+        return (isinstance(exc, (OperationalError,
+                                 extra_disconnection_errors)) and
                 exc.args[0] in (2006, 2013)) # (SERVER_GONE_ERROR, SERVER_LOST)
 
 
 
=== modified file 'storm/databases/postgres.py'
--- storm/databases/postgres.py	2011-03-21 18:59:55 +0000
+++ storm/databases/postgres.py	2011-09-28 06:23:18 +0000
@@ -276,9 +276,9 @@
             else:
                 yield param
 
-    def is_disconnection_error(self, exc):
+    def is_disconnection_error(self, exc, extra_disconnection_errors=()):
         if not isinstance(exc, (InterfaceError, OperationalError,
-                                ProgrammingError)):
+                                ProgrammingError, extra_disconnection_errors)):
             return False
 
         # XXX: 2007-09-17 jamesh
 
=== modified file 'storm/django/backend/base.py'
--- storm/django/backend/base.py	2011-07-14 11:36:04 +0000
+++ storm/django/backend/base.py	2011-09-28 06:23:18 +0000
@@ -18,6 +18,9 @@
     def _get_connection(self):
         if self._store is None:
             self._store = get_store(settings.DATABASE_NAME)
+        # Make sure that the store is registered with the transaction
+        # manager: we don't know what the connection will be used for.
+        self._store._event.emit("register-transaction")
         self._store._connection._ensure_connected()
         return self._store._connection._raw_connection
 
@@ -33,7 +36,6 @@
 
     def _cursor(self, *args):
         cursor = super(StormDatabaseWrapperMixin, self)._cursor(*args)
-        self._store._event.emit("register-transaction")
         return StormCursorWrapper(self._store, cursor)
 
     def _commit(self):
@@ -54,9 +56,14 @@
     """A cursor wrapper that checks for disconnection errors."""
 
     def __init__(self, store, cursor):
-        self._check_disconnect = store._connection._check_disconnect
+        self._connection = store._connection
         self._cursor = cursor
 
+    def _check_disconnect(self, *args, **kwargs):
+        from django.db import DatabaseError as DjangoDatabaseError
+        kwargs['extra_disconnection_errors'] = DjangoDatabaseError
+        return self._connection._check_disconnect(*args, **kwargs)
+
     def execute(self, statement, *args):
         """Execute an SQL statement."""
         return self._check_disconnect(self._cursor.execute, statement, *args)
 
=== modified file 'tests/database.py'
--- tests/database.py	2009-05-08 17:27:18 +0000
+++ tests/database.py	2011-09-28 06:23:18 +0000
@@ -236,7 +236,8 @@
 
         install_tracer(tracer_mock)
         self.connection.is_disconnection_error = (
-            lambda exc: 'connection closed' in str(exc))
+            lambda exc, extra_disconnection_errors=():
+                'connection closed' in str(exc))
 
         self.assertRaises(DisconnectionError,
                           self.connection.execute, "something")
@@ -251,7 +252,8 @@
 
         install_tracer(tracer_mock)
         self.connection.is_disconnection_error = (
-            lambda exc: 'connection closed' in str(exc))
+            lambda exc, extra_disconnection_errors=():
+                'connection closed' in str(exc))
 
         self.assertRaises(DisconnectionError,
                           self.connection.execute, "something")
@@ -270,7 +272,8 @@
 
         install_tracer(tracer_mock)
         self.connection.is_disconnection_error = (
-            lambda exc: 'connection closed' in str(exc))
+            lambda exc, extra_disconnection_errors=():
+                'connection closed' in str(exc))
 
         self.assertRaises(DisconnectionError,
                           self.connection.execute, "something")
@@ -356,7 +359,8 @@
         class FakeException(DatabaseError):
             """A fake database exception that indicates a disconnection."""
         self.connection.is_disconnection_error = (
-            lambda exc: isinstance(exc, FakeException))
+            lambda exc, extra_disconnection_errors=():
+                isinstance(exc, FakeException))
 
         self.assertEqual(self.connection._state,
                          storm.database.STATE_CONNECTED)
@@ -369,6 +373,32 @@
                          storm.database.STATE_DISCONNECTED)
         self.assertEqual(self.connection._raw_connection, None)
 
+    def test_wb_check_disconnection_extra_errors(self):
+        """Ensure that _check_disconnect() can check for additional
+        exceptions."""
+        class FakeException(DatabaseError):
+            """A fake database exception that indicates a disconnection."""
+        self.connection.is_disconnection_error = (
+            lambda exc, extra_disconnection_errors=():
+                isinstance(exc, extra_disconnection_errors))
+
+        self.assertEqual(self.connection._state,
+                         storm.database.STATE_CONNECTED)
+        # Error is converted to DisconnectionError:
+        def raise_exception():
+            raise FakeException
+        # Exception passes through as normal.
+        self.assertRaises(FakeException,
+                          self.connection._check_disconnect, raise_exception)
+        self.assertEqual(self.connection._state,
+                         storm.database.STATE_CONNECTED)
+        # Exception treated as a disconnection when keyword argument passed.
+        self.assertRaises(DisconnectionError,
+                          self.connection._check_disconnect, raise_exception,
+                          extra_disconnection_errors=FakeException)
+        self.assertEqual(self.connection._state,
+                         storm.database.STATE_DISCONNECTED)
+
     def test_wb_rollback_clears_disconnected_connection(self):
         """Check that rollback clears the DISCONNECTED state."""
         self.connection._state = storm.database.STATE_DISCONNECTED
 
=== modified file 'tests/django/backend.py'
--- tests/django/backend.py	2011-07-25 18:59:25 +0000
+++ tests/django/backend.py	2011-09-28 06:23:18 +0000
@@ -32,6 +32,7 @@
     from storm.django import stores
     from storm.zope.zstorm import global_zstorm, StoreDataManager
 
+import storm.database
 from storm.exceptions import DisconnectionError
 
 from tests.helper import TestHelper
@@ -146,6 +147,19 @@
         result = cursor.fetchall()
         self.assertEqual(len(result), 0)
 
+    def test_register_transaction(self):
+        wrapper = make_wrapper()
+        store = global_zstorm.get("django")
+        # Watch for register-transaction calls.
+        calls = []
+        def register_transaction(owner):
+            calls.append(owner)
+        store._event.hook("register-transaction", register_transaction)
+
+        cursor = wrapper.cursor()
+        cursor.execute("SELECT 1")
+        self.assertNotEqual(calls, [])
+
 
 class DjangoBackendDisconnectionTests(DatabaseDisconnectionMixin):
 
@@ -174,20 +188,58 @@
         transaction.manager.free(transaction.get())
         super(DjangoBackendDisconnectionTests, self).tearDown()
 
-    def test_disconnect(self):
-        from django.db import DatabaseError as DjangoDatabaseError
+    def test_wb_disconnect(self):
         wrapper = make_wrapper()
+        store = global_zstorm.get("django")
         cursor = wrapper.cursor()
         cursor.execute("SELECT 'about to reset connection'")
         wrapper._rollback()
+        cursor = wrapper.cursor()
         self.proxy.restart()
-        cursor = wrapper.cursor()
-        self.assertRaises((DjangoDatabaseError, DisconnectionError),
-                          cursor.execute, "SELECT 1")
-        wrapper._rollback()
-
-        cursor = wrapper.cursor()
-        cursor.execute("SELECT 1")
+        self.assertRaises(DisconnectionError, cursor.execute, "SELECT 1")
+        self.assertEqual(
+            store._connection._state, storm.database.STATE_DISCONNECTED)
+        wrapper._rollback()
+
+        self.assertEqual(
+            store._connection._state, storm.database.STATE_RECONNECT)
+        cursor = wrapper.cursor()
+        cursor.execute("SELECT 1")
+
+    def test_wb_transaction_registration(self):
+        wrapper = make_wrapper()
+        store = global_zstorm.get("django")
+        # Watch for register-transaction calls.
+        calls = []
+        def register_transaction(owner):
+            calls.append(owner)
+        store._event.hook("register-transaction", register_transaction)
+
+        # Simulate a disconnection, and put the connection into a
+        # state where it would attempt to reconnect.
+        store._connection._raw_connection = None
+        store._connection._state = storm.database.STATE_RECONNECT
+        self.proxy.stop()
+
+        self.assertRaises(DisconnectionError, wrapper.cursor)
+        # The connection is in the disconnected state, and has been
+        # registered with any listening transaction manager.
+        self.assertNotEqual(calls, [])
+        self.assertEqual(
+            store._connection._state, storm.database.STATE_DISCONNECTED)
+
+        wrapper._rollback()
+        del calls[:]
+
+        # Now reconnect:
+        self.proxy.start()
+        cursor = wrapper.cursor()
+        cursor.execute("SELECT 1")
+        # The connection is up, and has been registered with any
+        # listening transaction manager.
+        self.assertNotEqual(calls, [])
+        self.assertEqual(
+            store._connection._state, storm.database.STATE_CONNECTED)
 
 
 class PostgresDjangoBackendTests(DjangoBackendTests, TestHelper):