From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Tue, 15 May 2018 20:15:51 +0000 (-0400)
Subject: Prevent double-checkins and guard during reset-on-return invalidations
X-Git-Tag: rel_1_3_0b1~183^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=dada909a1009ad2f77063752ac8c13a7808dd916;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

Prevent double-checkins and guard during reset-on-return invalidations

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
---

diff --git a/doc/build/changelog/unreleased_12/4252.rst b/doc/build/changelog/unreleased_12/4252.rst
new file mode 100644
index 0000000000..7ca8c7d32d
--- /dev/null
+++ b/doc/build/changelog/unreleased_12/4252.rst
@@ -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
diff --git a/lib/sqlalchemy/pool.py b/lib/sqlalchemy/pool.py
index 89a4cea7c3..e22a682e0d 100644
--- a/lib/sqlalchemy/pool.py
+++ b/lib/sqlalchemy/pool.py
@@ -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()
 
 
diff --git a/test/engine/test_pool.py b/test/engine/test_pool.py
index b699afbc62..cdd0285489 100644
--- a/test/engine/test_pool.py
+++ b/test/engine/test_pool.py
@@ -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