Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove decreasing of created connections count when releasing not owned by connection pool connection(fixes issue #2832). #3514

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions redis/connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -1532,18 +1532,18 @@ def release(self, connection: "Connection") -> None:
except KeyError:
# Gracefully fail when a connection is returned to this pool
# that the pool doesn't actually own
pass
return

if self.owns_connection(connection):
self._available_connections.append(connection)
self._event_dispatcher.dispatch(
AfterConnectionReleasedEvent(connection)
)
else:
# pool doesn't own this connection. do not add it back
# to the pool and decrement the count so that another
# connection can take its place if needed
self._created_connections -= 1
# Pool doesn't own this connection, do not add it back
# to the pool.
# The created connections count shouls not be changed,
# because the connection was not created by the pool.
connection.disconnect()
return

Expand Down
15 changes: 15 additions & 0 deletions tests/test_connection_pool.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,21 @@ def test_reuse_previously_released_connection(self, master_host):
c2 = pool.get_connection()
assert c1 == c2

def test_release_not_owned_connection(self, master_host):
connection_kwargs = {"host": master_host[0], "port": master_host[1]}
pool1 = self.get_pool(connection_kwargs=connection_kwargs)
c1 = pool1.get_connection("_")
pool2 = self.get_pool(
connection_kwargs={"host": master_host[0], "port": master_host[1]}
)
c2 = pool2.get_connection("_")
pool2.release(c2)

assert len(pool2._available_connections) == 1

pool2.release(c1)
assert len(pool2._available_connections) == 1

def test_repr_contains_db_info_tcp(self):
connection_kwargs = {
"host": "localhost",
Expand Down
34 changes: 34 additions & 0 deletions tests/test_multiprocessing.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,40 @@ def target(conn, ev):
proc.join(3)
assert proc.exitcode == 0

@pytest.mark.parametrize("max_connections", [2, None])
def test_release_parent_connection_from_pool_in_child_process(
self, max_connections, master_host
):
"""
A connection owned by a parent should not decrease the _created_connections
counter in child when released - when the child process starts to use the
pool it resets all the counters that have been set in the parent process.
"""

pool = ConnectionPool.from_url(
f"redis://{master_host[0]}:{master_host[1]}",
max_connections=max_connections,
)

parent_conn = pool.get_connection("ping")

def target(pool, parent_conn):
with exit_callback(pool.disconnect):
child_conn = pool.get_connection("ping")
assert child_conn.pid != parent_conn.pid
pool.release(child_conn)
assert pool._created_connections == 1
assert child_conn in pool._available_connections
pool.release(parent_conn)
assert pool._created_connections == 1
assert child_conn in pool._available_connections
assert parent_conn not in pool._available_connections

proc = multiprocessing.Process(target=target, args=(pool, parent_conn))
proc.start()
proc.join(3)
assert proc.exitcode == 0

@pytest.mark.parametrize("max_connections", [1, 2, None])
def test_pool(self, max_connections, master_host):
"""
Expand Down
Loading