From 7c5b7d3e035859fd08946e031e62cc4abfe683f0 Mon Sep 17 00:00:00 2001
From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Fri, 14 Jul 2017 16:15:01 -0400
Subject: [PATCH] Don't access connection.info if connection is invalidated

The connection.info dictionary comes from the underlying
pooled connection, which in a disconnection situation
is not there; trying to access it makes it try to reconnect.
Inside of rollback(), SQLAlchemy doesn't allow us to
reconnect yet because we haven't finished removing the
transctional state that corresponds to the now-discarded
connection.  Therefore we have to check connection.invalidated
inside of our rollback handlers before trying to add/remove tokens
from connection.info.

Change-Id: Icd10ada68d6d53410ac88eca3577f04c5e30087e
Resolves-bug: #1704474
---
 oslo_db/sqlalchemy/exc_filters.py            | 12 ++++-
 oslo_db/tests/sqlalchemy/test_exc_filters.py | 49 ++++++++++++++++++++
 2 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/oslo_db/sqlalchemy/exc_filters.py b/oslo_db/sqlalchemy/exc_filters.py
index 2f575d8..9803343 100644
--- a/oslo_db/sqlalchemy/exc_filters.py
+++ b/oslo_db/sqlalchemy/exc_filters.py
@@ -485,7 +485,11 @@ def register_engine(engine):
     def rollback_savepoint(conn, name, context):
         exc_info = sys.exc_info()
         if exc_info[1]:
-            conn.info[ROLLBACK_CAUSE_KEY] = exc_info[1]
+            # NOTE(zzzeek) accessing conn.info on an invalidated
+            # connection causes it to reconnect, which we don't
+            # want to do inside a rollback handler
+            if not conn.invalidated:
+                conn.info[ROLLBACK_CAUSE_KEY] = exc_info[1]
         # NOTE(zzzeek) this eliminates a reference cycle between tracebacks
         # that would occur in Python 3 only, which has been shown to occur if
         # this function were in fact part of the traceback.  That's not the
@@ -497,7 +501,11 @@ def register_engine(engine):
     @event.listens_for(engine, "rollback")
     @event.listens_for(engine, "commit")
     def pop_exc_tx(conn):
-        conn.info.pop(ROLLBACK_CAUSE_KEY, None)
+        # NOTE(zzzeek) accessing conn.info on an invalidated
+        # connection causes it to reconnect, which we don't
+        # want to do inside a rollback handler
+        if not conn.invalidated:
+            conn.info.pop(ROLLBACK_CAUSE_KEY, None)
 
     # .. as well as connection pool checkin (just in case).
     # the .info dictionary lasts as long as the DBAPI connection itself
diff --git a/oslo_db/tests/sqlalchemy/test_exc_filters.py b/oslo_db/tests/sqlalchemy/test_exc_filters.py
index 558cca8..ce148db 100644
--- a/oslo_db/tests/sqlalchemy/test_exc_filters.py
+++ b/oslo_db/tests/sqlalchemy/test_exc_filters.py
@@ -712,6 +712,55 @@ class TestExceptionCauseMySQLSavepoint(test_base.MySQLOpportunisticTestCase):
         except exception.DBError as dbe_outer:
             self.assertIsNone(dbe_outer.cause)
 
+    def test_rollback_doesnt_interfere_with_killed_conn(self):
+        session = self.sessionmaker()
+
+        session.begin()
+        try:
+            session.execute("select 1")
+
+            # close underying DB connection
+            session.connection().connection.connection.close()
+
+            # alternate approach, but same idea:
+            # conn_id = session.scalar("select connection_id()")
+            # session.execute("kill connection %s" % conn_id)
+
+            # try using it, will raise an error
+            session.execute("select 1")
+        except exception.DBConnectionError:
+            # issue being tested is that this session.rollback()
+            # does not itself try to re-connect and raise another
+            # error.
+            session.rollback()
+        else:
+            assert False, "no exception raised"
+
+    def test_savepoint_rollback_doesnt_interfere_with_killed_conn(self):
+        session = self.sessionmaker()
+
+        session.begin()
+        try:
+            session.begin_nested()
+            session.execute("select 1")
+
+            # close underying DB connection
+            session.connection().connection.connection.close()
+
+            # alternate approach, but same idea:
+            # conn_id = session.scalar("select connection_id()")
+            # session.execute("kill connection %s" % conn_id)
+
+            # try using it, will raise an error
+            session.execute("select 1")
+        except exception.DBConnectionError:
+            # issue being tested is that this session.rollback()
+            # does not itself try to re-connect and raise another
+            # error.
+            session.rollback()
+        else:
+            assert False, "no exception raised"
+
 
 class TestDBDataErrorSQLite(_SQLAExceptionMatcher, test_base.DbTestCase):