Fix ConnectionPool to raise MaxConnectionsError instead of Connection… by ngabhanenetskope · Pull Request #3698 · redis/redis-py (original) (raw)

Pull Request check-list

Please make sure to review and check all of these items:

• [✅] Do tests and lints pass with this change?
• [✅] Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
• [✅] Is the new or changed code fully tested?
• [✅] Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
• Is there an example added to the examples folder (if applicable)? (N/A - This is an error class enhancement)

Description of change

This PR addresses issue #3684 by introducing a more specific MaxConnectionsError exception that is raised when a connection pool reaches its maximum connections limit.

Current behavior:
When a ConnectionPool reaches its maximum connections limit, it raises a generic ConnectionError. This causes RedisCluster to treat the error as a node failure and trigger an unnecessary cluster reinitialization, leading to increased resource usage.

Changes:

1. Added a new MaxConnectionsError class in redis/exceptions.py as a subclass of ConnectionError
2. Modified ConnectionPool.make_connection() in redis/connection.py to raise the new MaxConnectionsError instead of a generic ConnectionError
3. Updated RedisCluster._execute_command() in redis/cluster.py to handle MaxConnectionsError separately, preventing unnecessary cluster reinitialization
4. Updated tests to verify the new behavior in both standalone and cluster modes
5. Added exports in __init__.py to make the new exception available

Testing:

1. Updated test_max_connections in test_connection_pool.py to expect MaxConnectionsError
2. Added a new test file test_max_connections_error.py with:
   • Inheritance test to verify MaxConnectionsError extends ConnectionError
   • Connection pool test to verify the correct error is raised
   • Mock-based cluster test to verify cluster doesn't reinitialize on MaxConnectionsError

All tests pass locally, including the new tests specifically for this behavior.

Note: The issue was previously assigned but appears to be inactive. I implemented this fix because it was affecting our production environment.