From 85d1899b76a37b4bf922b1cea4f608ba806b41d0 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Sat, 10 May 2014 15:31:49 -0400 Subject: [PATCH] - Fixed some "double invalidate" situations were detected where a connection invalidation could occur within an already critical section like a connection.close(); ultimately, these conditions are caused by the change in :ticket:`2907`, in that the "reset on return" feature calls out to the Connection/Transaction in order to handle it, where "disconnect detection" might be caught. However, it's possible that the more recent change in :ticket:`2985` made it more likely for this to be seen as the "connection invalidate" operation is much quicker, as the issue is more reproducible on 0.9.4 than 0.9.3. Checks are now added within any section that an invalidate might occur to halt further disallowed operations on the invalidated connection. This includes two fixes both at the engine level and at the pool level. While the issue was observed with highly concurrent gevent cases, it could in theory occur in any kind of scenario where a disconnect occurs within the connection close operation. fixes #3043 ref #2985 ref #2907 - add some defensive checks during an invalidate situation: 1. _ConnectionRecord.invalidate might be called twice within finalize_fairy if the _reset() raises an invalidate condition, invalidates, raises and then goes to invalidate the CR. so check for this. 2. similarly within Conneciton, anytime we do handle_dbapi_error(), we might become invalidated. so a following finally must check self.__invalid before dealing with the connection any futher. --- doc/build/changelog/changelog_09.rst | 22 ++++++++++++++++++++++ lib/sqlalchemy/engine/base.py | 13 ++++++++++--- lib/sqlalchemy/pool.py | 4 ++++ 3 files changed, 36 insertions(+), 3 deletions(-) diff --git a/doc/build/changelog/changelog_09.rst b/doc/build/changelog/changelog_09.rst index cd8cb0ac16..14ef6a601c 100644 --- a/doc/build/changelog/changelog_09.rst +++ b/doc/build/changelog/changelog_09.rst @@ -14,6 +14,28 @@ .. changelog:: :version: 0.9.5 + .. change:: + :tags: bug, engine + :tickets: 3043 + + Fixed some "double invalidate" situations were detected where + a connection invalidation could occur within an already critical section + like a connection.close(); ultimately, these conditions are caused + by the change in :ticket:`2907`, in that the "reset on return" feature + calls out to the Connection/Transaction in order to handle it, where + "disconnect detection" might be caught. However, it's possible that + the more recent change in :ticket:`2985` made it more likely for this + to be seen as the "connection invalidate" operation is much quicker, + as the issue is more reproducible on 0.9.4 than 0.9.3. + + Checks are now added within any section that + an invalidate might occur to halt further disallowed operations + on the invalidated connection. This includes two fixes both at the + engine level and at the pool level. While the issue was observed + with highly concurrent gevent cases, it could in theory occur in + any kind of scenario where a disconnect occurs within the connection + close operation. + .. change:: :tags: feature, orm :tickets: 3029 diff --git a/lib/sqlalchemy/engine/base.py b/lib/sqlalchemy/engine/base.py index bb3b82eea0..997991b300 100644 --- a/lib/sqlalchemy/engine/base.py +++ b/lib/sqlalchemy/engine/base.py @@ -507,7 +507,8 @@ class Connection(Connectable): except Exception as e: self._handle_dbapi_exception(e, None, None, None, None) finally: - if self.connection._reset_agent is self.__transaction: + if not self.__invalid and \ + self.connection._reset_agent is self.__transaction: self.connection._reset_agent = None self.__transaction = None else: @@ -524,7 +525,8 @@ class Connection(Connectable): except Exception as e: self._handle_dbapi_exception(e, None, None, None, None) finally: - if self.connection._reset_agent is self.__transaction: + if not self.__invalid and \ + self.connection._reset_agent is self.__transaction: self.connection._reset_agent = None self.__transaction = None @@ -637,7 +639,12 @@ class Connection(Connectable): conn.close() if conn._reset_agent is self.__transaction: conn._reset_agent = None - del self.__connection + + # the close() process can end up invalidating us, + # as the pool will call our transaction as the "reset_agent" + # for rollback(), which can then cause an invalidation + if not self.__invalid: + del self.__connection self.__can_reconnect = False self.__transaction = None diff --git a/lib/sqlalchemy/pool.py b/lib/sqlalchemy/pool.py index 799443546f..4020311a00 100644 --- a/lib/sqlalchemy/pool.py +++ b/lib/sqlalchemy/pool.py @@ -479,6 +479,9 @@ class _ConnectionRecord(object): :ref:`pool_connection_invalidation` """ + # already invalidated + if self.connection is None: + return self.__pool.dispatch.invalidate(self.connection, self, e) if e is not None: self.__pool.logger.info( @@ -557,6 +560,7 @@ def _finalize_fairy(connection, connection_record, pool, ref, echo, fairy=None): if not connection_record: pool._close_connection(connection) except Exception as e: + pool.logger.error("Exception during reset or similar", exc_info=True) if connection_record: connection_record.invalidate(e=e) if isinstance(e, (SystemExit, KeyboardInterrupt)): -- 2.47.3