From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Fri, 30 May 2014 20:24:38 +0000 (-0400)
Subject: - vastly improve the "safe close cursor" tests in test_reconnect
X-Git-Tag: rel_0_9_5~19
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=758dc17f4fd057a96f0a9d8f90e05bbd9537fc67;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

- vastly improve the "safe close cursor" tests in test_reconnect
- Fixed bug which would occur if a DBAPI exception
occurs when the engine first connects and does its initial checks,
and the exception is not a disconnect exception, yet the cursor
raises an error when we try to close it.  In this case the real
exception would be quashed as we tried to log the cursor close
exception via the connection pool and failed, as we were trying
to access the pool's logger in a way that is inappropriate
in this very specific scenario. fixes #3063
---

diff --git a/doc/build/changelog/changelog_09.rst b/doc/build/changelog/changelog_09.rst
index 5d39f1b965..2346151994 100644
--- a/doc/build/changelog/changelog_09.rst
+++ b/doc/build/changelog/changelog_09.rst
@@ -14,6 +14,20 @@
 .. changelog::
     :version: 0.9.5
 
+    .. change::
+        :tags: bug, engine
+        :versions: 1.0.0
+        :tickets: 3063
+
+        Fixed bug which would occur if a DBAPI exception
+        occurs when the engine first connects and does its initial checks,
+        and the exception is not a disconnect exception, yet the cursor
+        raises an error when we try to close it.  In this case the real
+        exception would be quashed as we tried to log the cursor close
+        exception via the connection pool and failed, as we were trying
+        to access the pool's logger in a way that is inappropriate
+        in this very specific scenario.
+
     .. change::
         :tags: feature, postgresql
         :versions: 1.0.0
diff --git a/lib/sqlalchemy/engine/base.py b/lib/sqlalchemy/engine/base.py
index f9fc04d760..249c494fe1 100644
--- a/lib/sqlalchemy/engine/base.py
+++ b/lib/sqlalchemy/engine/base.py
@@ -1054,8 +1054,9 @@ class Connection(Connectable):
         except (SystemExit, KeyboardInterrupt):
             raise
         except Exception:
-            self.connection._logger.error(
-                                    "Error closing cursor", exc_info=True)
+            # log the error through the connection pool's logger.
+            self.engine.pool.logger.error(
+                                "Error closing cursor", exc_info=True)
 
     _reentrant_error = False
     _is_disconnect = False
diff --git a/lib/sqlalchemy/engine/strategies.py b/lib/sqlalchemy/engine/strategies.py
index a8a63bb3d5..691c06a8c3 100644
--- a/lib/sqlalchemy/engine/strategies.py
+++ b/lib/sqlalchemy/engine/strategies.py
@@ -161,7 +161,6 @@ class DefaultEngineStrategy(EngineStrategy):
             def first_connect(dbapi_connection, connection_record):
                 c = base.Connection(engine, connection=dbapi_connection,
                             _has_events=False)
-
                 dialect.initialize(c)
             event.listen(pool, 'first_connect', first_connect, once=True)
 
diff --git a/test/engine/test_reconnect.py b/test/engine/test_reconnect.py
index 23a3b37037..e6897b13d6 100644
--- a/test/engine/test_reconnect.py
+++ b/test/engine/test_reconnect.py
@@ -373,33 +373,76 @@ class MockReconnectTest(fixtures.TestBase):
 
 
 class CursorErrTest(fixtures.TestBase):
+    # this isn't really a "reconnect" test, it's more of
+    # a generic "recovery".   maybe this test suite should have been
+    # named "test_error_recovery".
+    def _fixture(self, explode_on_exec, initialize):
+        class DBAPIError(Exception):
+            pass
 
-    def setup(self):
         def MockDBAPI():
             def cursor():
                 while True:
-                    yield Mock(
-                        description=[],
-                        close=Mock(side_effect=Exception("explode")))
+                    if explode_on_exec:
+                        yield Mock(
+                            description=[],
+                            close=Mock(side_effect=DBAPIError("explode")),
+                            execute=Mock(side_effect=DBAPIError("explode"))
+                        )
+                    else:
+                        yield Mock(
+                            description=[],
+                            close=Mock(side_effect=Exception("explode")),
+                        )
             def connect():
                 while True:
-                    yield Mock(cursor=Mock(side_effect=cursor()))
-
-            return Mock(connect=Mock(side_effect=connect()))
-
+                    yield Mock(
+                                spec=['cursor', 'commit', 'rollback', 'close'],
+                                cursor=Mock(side_effect=cursor()),
+                            )
+
+            return Mock(
+                        Error = DBAPIError,
+                        paramstyle='qmark',
+                        connect=Mock(side_effect=connect())
+                    )
         dbapi = MockDBAPI()
-        self.db = testing_engine(
-                    'postgresql://foo:bar@localhost/test',
-                    options=dict(module=dbapi, _initialize=False))
+
+        from sqlalchemy.engine import default
+        url = Mock(
+                    get_dialect=lambda: default.DefaultDialect,
+                    translate_connect_args=lambda: {},
+                    query={},
+                )
+        eng = testing_engine(
+                    url,
+                    options=dict(module=dbapi, _initialize=initialize))
+        eng.pool.logger = Mock()
+        return eng
 
     def test_cursor_explode(self):
-        conn = self.db.connect()
+        db = self._fixture(False, False)
+        conn = db.connect()
         result = conn.execute("select foo")
         result.close()
         conn.close()
+        eq_(
+            db.pool.logger.error.mock_calls,
+            [call('Error closing cursor', exc_info=True)]
+        )
+
+    def test_cursor_shutdown_in_initialize(self):
+        db = self._fixture(True, True)
+        assert_raises_message(
+            exc.SAWarning,
+            "Exception attempting to detect",
+            db.connect
+        )
+        eq_(
+            db.pool.logger.error.mock_calls,
+            [call('Error closing cursor', exc_info=True)]
+        )
 
-    def teardown(self):
-        self.db.dispose()
 
 
 def _assert_invalidated(fn, *args):