connectionrec recycle can create situation where checkout handler is called w/o connect handler being successful
Issue #3497
resolved
diff --git a/lib/sqlalchemy/pool.py b/lib/sqlalchemy/pool.py
index b38aefb..9af956b 100644
--- a/lib/sqlalchemy/pool.py
+++ b/lib/sqlalchemy/pool.py
@@ -453,6 +453,7 @@ class _ConnectionRecord(object):
for_modify(pool.dispatch).\
exec_once(self.connection, self)
pool.dispatch.connect(self.connection, self)
+ self.info['x'] = 1
connection = None
"""A reference to the actual DBAPI connection being tracked.
@@ -563,6 +564,8 @@ class _ConnectionRecord(object):
self.connection = self.__connect()
if self.__pool.dispatch.connect:
self.__pool.dispatch.connect(self.connection, self)
+ self.info['x'] = 1
+
elif self.__pool._recycle > -1 and \
time.time() - self.starttime > self.__pool._recycle:
self.__pool.logger.info(
@@ -587,9 +590,17 @@ class _ConnectionRecord(object):
if recycle:
self.__close()
self.info.clear()
+
+ # this is the fix; ensure self.connection is None first
+ # self.connection = None
+
+ # here, if __connect() fails, self.connection
+ # is still referring to the old connection, but info is empty
+ # and we won't attempt a __connect() next time we are here
self.connection = self.__connect()
if self.__pool.dispatch.connect:
self.__pool.dispatch.connect(self.connection, self)
+ self.info['x'] = 1
return self.connection
def __close(self):
@@ -718,12 +729,14 @@ class _ConnectionFairy(object):
fairy._counter += 1
if not pool.dispatch.checkout or fairy._counter != 1:
+ assert 'x' in fairy.info
return fairy
# Pool listeners can trigger a reconnection on checkout
attempts = 2
while attempts > 0:
try:
+ assert fairy.info['x']
pool.dispatch.checkout(fairy.connection,
fairy._connection_record,
fairy)
then run:
py.test test/engine/test_pool.py -k test_error_on_pooled_reconnect_cleanup_recycle
this warrants an immediate 1.0.8 IMO, I would like to try to produce an oslo.db-level reproduction case though
Comments (3)
-
reporter -
reporter - changed status to resolved
- Fixed critical issue whereby the pool "checkout" event handler
may be called against a stale connection without the "connect"
event handler having been called, in the case where the pool
attempted to reconnect after being invalidated and failed; the stale
connection would remain present and would be used on a subsequent
attempt. This issue has a greater impact in the 1.0 series subsequent
to 1.0.2, as it also delivers a blanked-out
.info
dictionary to the event handler; prior to 1.0.2 the.info
dictionary is still the previous one. fixes#3497
→ <<cset 4096ad0f0980>>
-
reporter - Fixed critical issue whereby the pool "checkout" event handler
may be called against a stale connection without the "connect"
event handler having been called, in the case where the pool
attempted to reconnect after being invalidated and failed; the stale
connection would remain present and would be used on a subsequent
attempt. This issue has a greater impact in the 1.0 series subsequent
to 1.0.2, as it also delivers a blanked-out
.info
dictionary to the event handler; prior to 1.0.2 the.info
dictionary is still the previous one. fixes#3497
(cherry picked from commit 4096ad0f0980f6940be57aaee85791df8f975cd7)
→ <<cset 0aee8eb0015b>>
- Fixed critical issue whereby the pool "checkout" event handler
may be called against a stale connection without the "connect"
event handler having been called, in the case where the pool
attempted to reconnect after being invalidated and failed; the stale
connection would remain present and would be used on a subsequent
attempt. This issue has a greater impact in the 1.0 series subsequent
to 1.0.2, as it also delivers a blanked-out
- Log in to comment
so what's interesting to note here is that while the issue probably exists in 0.9 as well, we have a different system of resetting the ".info" there where we do it after the connect attempt. We've moved that up to suit the HAAlchemy use case in 1.0. but this is an 0.9 issue regardless so when we test we should find a test that looks at it from the perspective of the connection events themselves and not necessarily .info.