Skip to content

Commit d867660

Browse files
authored
Remove decreasing of created connections count when releasing not owned by connection pool connection(fixes issue #2832). (#3514)
* Removing decreasing of created connections count when releasing not owned by connection pool connection(#2832). * Fixed another issue that was allowing adding connections to a pool owned by other pools. Adding unit tests. * Fixing a typo in a comment
1 parent 75cac31 commit d867660

File tree

3 files changed

+54
-5
lines changed

3 files changed

+54
-5
lines changed

redis/connection.py

+5-5
Original file line numberDiff line numberDiff line change
@@ -1532,18 +1532,18 @@ def release(self, connection: "Connection") -> None:
15321532
except KeyError:
15331533
# Gracefully fail when a connection is returned to this pool
15341534
# that the pool doesn't actually own
1535-
pass
1535+
return
15361536

15371537
if self.owns_connection(connection):
15381538
self._available_connections.append(connection)
15391539
self._event_dispatcher.dispatch(
15401540
AfterConnectionReleasedEvent(connection)
15411541
)
15421542
else:
1543-
# pool doesn't own this connection. do not add it back
1544-
# to the pool and decrement the count so that another
1545-
# connection can take its place if needed
1546-
self._created_connections -= 1
1543+
# Pool doesn't own this connection, do not add it back
1544+
# to the pool.
1545+
# The created connections count should not be changed,
1546+
# because the connection was not created by the pool.
15471547
connection.disconnect()
15481548
return
15491549

tests/test_connection_pool.py

+15
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,21 @@ def test_reuse_previously_released_connection(self, master_host):
9191
c2 = pool.get_connection()
9292
assert c1 == c2
9393

94+
def test_release_not_owned_connection(self, master_host):
95+
connection_kwargs = {"host": master_host[0], "port": master_host[1]}
96+
pool1 = self.get_pool(connection_kwargs=connection_kwargs)
97+
c1 = pool1.get_connection("_")
98+
pool2 = self.get_pool(
99+
connection_kwargs={"host": master_host[0], "port": master_host[1]}
100+
)
101+
c2 = pool2.get_connection("_")
102+
pool2.release(c2)
103+
104+
assert len(pool2._available_connections) == 1
105+
106+
pool2.release(c1)
107+
assert len(pool2._available_connections) == 1
108+
94109
def test_repr_contains_db_info_tcp(self):
95110
connection_kwargs = {
96111
"host": "localhost",

tests/test_multiprocessing.py

+34
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,40 @@ def target(conn, ev):
8484
proc.join(3)
8585
assert proc.exitcode == 0
8686

87+
@pytest.mark.parametrize("max_connections", [2, None])
88+
def test_release_parent_connection_from_pool_in_child_process(
89+
self, max_connections, master_host
90+
):
91+
"""
92+
A connection owned by a parent should not decrease the _created_connections
93+
counter in child when released - when the child process starts to use the
94+
pool it resets all the counters that have been set in the parent process.
95+
"""
96+
97+
pool = ConnectionPool.from_url(
98+
f"redis://{master_host[0]}:{master_host[1]}",
99+
max_connections=max_connections,
100+
)
101+
102+
parent_conn = pool.get_connection("ping")
103+
104+
def target(pool, parent_conn):
105+
with exit_callback(pool.disconnect):
106+
child_conn = pool.get_connection("ping")
107+
assert child_conn.pid != parent_conn.pid
108+
pool.release(child_conn)
109+
assert pool._created_connections == 1
110+
assert child_conn in pool._available_connections
111+
pool.release(parent_conn)
112+
assert pool._created_connections == 1
113+
assert child_conn in pool._available_connections
114+
assert parent_conn not in pool._available_connections
115+
116+
proc = multiprocessing.Process(target=target, args=(pool, parent_conn))
117+
proc.start()
118+
proc.join(3)
119+
assert proc.exitcode == 0
120+
87121
@pytest.mark.parametrize("max_connections", [1, 2, None])
88122
def test_pool(self, max_connections, master_host):
89123
"""

0 commit comments

Comments
 (0)