]> git.ipfire.org Git - thirdparty/sqlalchemy/sqlalchemy.git/commitdiff
Prevent double-checkins and guard during reset-on-return invalidations
authorMike Bayer <mike_mp@zzzcomputing.com>
Tue, 15 May 2018 20:15:51 +0000 (16:15 -0400)
committerMike Bayer <mike_mp@zzzcomputing.com>
Tue, 15 May 2018 20:16:59 +0000 (16:16 -0400)
Fixed connection pool issue whereby if a disconnection error were raised
during the connection pool's "reset on return" sequence in conjunction with
an explicit transaction opened against the enclosing :class:`.Connection`
object (such as from calling :meth:`.Session.close` without a rollback or
commit, or calling :meth:`.Connection.close` without first closing a
transaction declared with :meth:`.Connection.begin`), a double-checkin would
result, which could then lead towards concurrent checkouts of the same
connection. The double-checkin condition is now prevented overall by an
assertion, as well as the specific double-checkin scenario has been
fixed.

Change-Id: If5bb6941e36326846b14918c33ebfdd5604f642e
Fixes: #4252
doc/build/changelog/unreleased_12/4252.rst [new file with mode: 0644]
lib/sqlalchemy/pool.py
test/engine/test_pool.py

diff --git a/doc/build/changelog/unreleased_12/4252.rst b/doc/build/changelog/unreleased_12/4252.rst
new file mode 100644 (file)
index 0000000..7ca8c7d
--- /dev/null
@@ -0,0 +1,15 @@
+.. change::
+    :tags: bug, engine
+    :tickets: 4252
+    :versions: 1.3.0b1
+
+    Fixed connection pool issue whereby if a disconnection error were raised
+    during the connection pool's "reset on return" sequence in conjunction with
+    an explicit transaction opened against the enclosing :class:`.Connection`
+    object (such as from calling :meth:`.Session.close` without a rollback or
+    commit, or calling :meth:`.Connection.close` without first closing a
+    transaction declared with :meth:`.Connection.begin`), a double-checkin would
+    result, which could then lead towards concurrent checkouts of the same
+    connection. The double-checkin condition is now prevented overall by an
+    assertion, as well as the specific double-checkin scenario has been
+    fixed.
\ No newline at end of file
index 89a4cea7c303b193975349f973ab66e02c1e74b5..e22a682e0d572a4e84f511f457347cded979336e 100644 (file)
@@ -552,9 +552,12 @@ class _ConnectionRecord(object):
 
     def _checkin_failed(self, err):
         self.invalidate(e=err)
-        self.checkin()
+        self.checkin(_no_fairy_ref=True)
 
-    def checkin(self):
+    def checkin(self, _no_fairy_ref=False):
+        if self.fairy_ref is None and not _no_fairy_ref:
+            util.warn("Double checkin attempted on %s" % self)
+            return
         self.fairy_ref = None
         connection = self.connection
         pool = self.__pool
@@ -721,7 +724,7 @@ def _finalize_fairy(connection, connection_record,
             if not isinstance(e, Exception):
                 raise
 
-    if connection_record:
+    if connection_record and connection_record.fairy_ref is not None:
         connection_record.checkin()
 
 
index b699afbc62c9879293b271d44d999e90872900fb..cdd02854895fb99d279d7304d8abca6a37c81eba 100644 (file)
@@ -7,6 +7,7 @@ from sqlalchemy.testing.util import gc_collect, lazy_gc
 from sqlalchemy.testing import eq_, assert_raises, is_not_, is_
 from sqlalchemy.testing.engines import testing_engine
 from sqlalchemy.testing import fixtures
+from sqlalchemy.testing import assert_raises_message
 import random
 from sqlalchemy.testing.mock import Mock, call, patch, ANY
 import weakref
@@ -1970,6 +1971,18 @@ class QueuePoolTest(PoolTestBase):
         c2 = p.connect()
         assert c2.connection is not None
 
+    def test_no_double_checkin(self):
+        p = self._queuepool_fixture(pool_size=1)
+
+        c1 = p.connect()
+        rec = c1._connection_record
+        c1.close()
+        assert_raises_message(
+            Warning,
+            "Double checkin attempted on %s" % rec,
+            rec.checkin
+        )
+
 
 class ResetOnReturnTest(PoolTestBase):
     def _fixture(self, **kw):
@@ -2063,6 +2076,27 @@ class ResetOnReturnTest(PoolTestBase):
         assert not dbapi.connect().rollback.called
         assert dbapi.connect().commit.called
 
+    def test_reset_agent_disconnect(self):
+        dbapi, p = self._fixture(reset_on_return='rollback')
+
+        class Agent(object):
+            def __init__(self, conn):
+                self.conn = conn
+
+            def rollback(self):
+                p._invalidate(self.conn)
+                raise Exception("hi")
+
+            def commit(self):
+                self.conn.commit()
+
+        c1 = p.connect()
+        c1._reset_agent = Agent(c1)
+        c1.close()
+
+        # no warning raised.  We know it would warn due to
+        # QueuePoolTest.test_no_double_checkin
+
 
 class SingletonThreadPoolTest(PoolTestBase):
     @testing.requires.threading_with_mock