Merge lp:~salgado/storm/bug-374909 into lp:storm

[Sidnei da Silva]

[Jamu Kakar (jkakar)]

Looks good to me, +1!

review: Approve

[James Henstridge (jamesh)]

Looks good.

review: Approve

[James Henstridge (jamesh)]

Actually, it needs a bit of work. I got a test failure when trying to merge your branch:

======================================================================
FAIL: Test that InterfaceErrors get caught on rollback().
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/james/src/storm/storm.trunk/tests/mocker.py", line 102, in test_method_wrapper
    result = test_method()
  File "/home/james/src/storm/storm.trunk/tests/databases/base.py", line 549, in test_rollback_swallows_InterfaceError
    self.fail('Exception should have been swallowed: %s' % repr(exc))
AssertionError: Exception should have been swallowed: InterfaceError('connection already closed',)

----------------------------------------------------------------------

That test is too specific to the PostgreSQL backend.

I'd suggest modifying the test to be closer to test_wb_catch_already_disconnected(), but replace the final assertRaises() call with the connection.rollback() call from your test.

That will test the behaviour you are after (handling the case of rollback on a connection where we missed the initial disconnection notification) rather than simply testing that this one particular exception is caught.

review: Needs Fixing

[James Henstridge (jamesh)]

The above failure is for the MySQL backend, btw.

[Guilherme Salgado (salgado)]

On Wed, 2009-07-29 at 08:57 +0000, James Henstridge wrote:
[...]
> That test is too specific to the PostgreSQL backend.
>
> I'd suggest modifying the test to be closer to test_wb_catch_already_disconnected(), but replace the final assertRaises() call with the connection.rollback() call from your test.

That sounds good, but such test would not fail if I reverted my changes,
so I thought of moving my original test to PostgresDisconnectionTest and
writing a new one as you suggest on DatabaseDisconnectionTest.

I've pushed these changes up. Please let me know what you think

--
Guilherme Salgado <email address hidden>

[lp:~salgado/storm/bug-374909]

317. By Guilherme Salgado on 2009-07-29

Move original test to PostgresDisconnectionTest as it is postgres-specific and write a new generic one on DatabaseDisconnectionTest

[James Henstridge (jamesh)]

> That sounds good, but such test would not fail if I reverted my changes,
> so I thought of moving my original test to PostgresDisconnectionTest and
> writing a new one as you suggest on DatabaseDisconnectionTest.

It seems that that the test doesn't fail on MySQL due to MySQL issuing a DatabaseError subclass in this situation. So that indicates that this particular bug was only affecting the PostgreSQL backend.

That doesn't make the test useless though: it makes sure we don't reintroduce the bug, or have it occur in future backends.

> I've pushed these changes up. Please let me know what you think

They look good. I've merged them as r319.

review: Approve

[Guilherme Salgado (salgado)]

On Thu, 2009-07-30 at 06:23 +0000, James Henstridge wrote:
> Review: Approve
> > That sounds good, but such test would not fail if I reverted my changes,
> > so I thought of moving my original test to PostgresDisconnectionTest and
> > writing a new one as you suggest on DatabaseDisconnectionTest.
>
> It seems that that the test doesn't fail on MySQL due to MySQL issuing a DatabaseError subclass in this situation. So that indicates that this particular bug was only affecting the PostgreSQL backend.
>
> That doesn't make the test useless though: it makes sure we don't reintroduce the bug, or have it occur in future backends.
>

Absolutely; I was just arguing for a test that was specific for the
InterfaceError exception.

>
> > I've pushed these changes up. Please let me know what you think
>
> They look good. I've merged them as r319.

Thanks!

--
Guilherme Salgado <email address hidden>

=== modified file 'storm/database.py'
--- storm/database.py	2009-07-29 05:21:32 +0000
+++ storm/database.py	2009-07-29 16:20:10 +0000
@@ -243,7 +243,7 @@
         if self._state == STATE_CONNECTED:
             try:
                 self._raw_connection.rollback()
-            except DatabaseError, exc:
+            except Error, exc:
                 if self.is_disconnection_error(exc):
                     self._raw_connection = None
                     self._state = STATE_RECONNECT

=== modified file 'tests/databases/base.py'
--- tests/databases/base.py	2009-07-29 05:21:32 +0000
+++ tests/databases/base.py	2009-07-29 16:20:10 +0000
@@ -26,14 +26,15 @@
 import os
 
 from storm.uri import URI
-from storm.expr import Select, Column, Undef, SQLToken, SQLRaw, Count, Alias
+from storm.expr import Select, Column, SQLToken, SQLRaw, Count, Alias
 from storm.variables import (Variable, PickleVariable, RawStrVariable,
                              DecimalVariable, DateTimeVariable, DateVariable,
                              TimeVariable, TimeDeltaVariable)
 from storm.database import *
 from storm.event import EventSystem
 from storm.exceptions import (
-    DatabaseError, DatabaseModuleError, DisconnectionError, OperationalError)
+    DatabaseError, DatabaseModuleError, DisconnectionError, Error,
+    OperationalError)
 
 from tests.databases.proxy import ProxyTCPServer
 from tests.helper import MakePath
@@ -532,6 +533,39 @@
         self.proxy.restart()
         self.assertRaises(DisconnectionError, self.connection.commit)
 
+    def test_wb_catch_already_disconnected_on_rollback(self):
+        """Connection.rollback() swallows disconnection errors.
+
+        If the connection is being used outside of Storm's control,
+        then it is possible that Storm won't see the disconnection.
+        It should be able to recover from this situation though.
+        """
+        result = self.connection.execute("SELECT 1")
+        self.assertTrue(result.get_one())
+        self.proxy.restart()
+        # Perform an action that should result in a disconnection.
+        try:
+            cursor = self.connection._raw_connection.cursor()
+            cursor.execute("SELECT 1")
+            cursor.fetchone()
+        except Error, exc:
+            self.assertTrue(self.connection.is_disconnection_error(exc))
+        else:
+            self.fail("Disconnection was not caught.")
+
+        # Make sure our raw connection's rollback() raises a disconnection
+        # error when called.
+        try:
+            self.connection._raw_connection.rollback()
+        except Error, exc:
+            self.assertTrue(self.connection.is_disconnection_error(exc))
+        else:
+            self.fail("Disconnection was not raised.")
+
+        # Our rollback() will catch and swallow that disconnection error,
+        # though.
+        self.connection.rollback()
+
     def test_wb_catch_already_disconnected(self):
         """Storm detects connections that have already been disconnected.
 

=== modified file 'tests/databases/postgres.py'
--- tests/databases/postgres.py	2008-10-02 15:47:18 +0000
+++ tests/databases/postgres.py	2009-07-29 16:20:10 +0000
@@ -24,7 +24,7 @@
 from storm.databases.postgres import (
     Postgres, compile, currval, Returning, PostgresTimeoutTracer)
 from storm.database import create_database
-from storm.exceptions import ProgrammingError
+from storm.exceptions import InterfaceError, ProgrammingError
 from storm.variables import DateTimeVariable, RawStrVariable
 from storm.variables import ListVariable, IntVariable, Variable
 from storm.properties import Int
@@ -526,6 +526,21 @@
     host_environment_variable = "STORM_POSTGRES_HOST_URI"
     default_port = 5432
 
+    def test_rollback_swallows_InterfaceError(self):
+        """Test that InterfaceErrors get caught on rollback().
+
+        InterfaceErrors are a form of a disconnection error, so rollback()
+        must swallow them and reconnect.
+        """
+        class FakeConnection:
+            def rollback(self):
+                raise InterfaceError('connection already closed')
+        self.connection._raw_connection = FakeConnection()
+        try:
+            self.connection.rollback()
+        except Exception, exc:
+            self.fail('Exception should have been swallowed: %s' % repr(exc))
+
 
 class PostgresTimeoutTracerTest(TimeoutTracerTestBase):