"No connection available" errors since 5.0.1

[DPham-X]

Version: 5.0.1

Platform:
Python 3.11.5(3.11.5 (main, Sep 4 2023, 15:30:52) [GCC 10.2.1 20210110])

Description:

Since 5.0.1, we noticed a significant increase in No connection available. errors from redis. We are using cashews (6.3.0) + redis-py for request caching for our fastapi application. We don't experience this with 5.0.0.

Stack trace:

CancelledError: null
  File "redis/asyncio/connection.py", line 1170, in get_connection
    async with self._condition:
  File "asyncio/locks.py", line 15, in __aenter__
    await self.acquire()
  File "asyncio/locks.py", line 114, in acquire
    await fut
TimeoutError: null
  File "redis/asyncio/connection.py", line 1169, in get_connection
    async with async_timeout(self.timeout):
  File "asyncio/timeouts.py", line 111, in __aexit__
    raise TimeoutError from exc_val
ConnectionError: No connection available.
  File "cashews/backends/redis/client.py", line 26, in execute_command
    return await super().execute_command(command, *args, **kwargs)
  File "redis/asyncio/client.py", line 601, in execute_command
    conn = self.connection or await pool.get_connection(command_name, **options)
  File "redis/asyncio/connection.py", line 1174, in get_connection
    raise ConnectionError("No connection available.") from err

[paul-finary]

Same here, I have cases where "No connection available." is raised but the pool has 4 _available_connections, and only 5 _in_use_connections out of 500 max_connections.

@kristjanvalur I saw you refactored the ConnectionPool / BlockingConnectionPool in 5.0.1, do you have any idea of what would trigger this ?

[kristjanvalur]

async Redis client and connection pools now need to be properly closed.
The bug is in cachews. While the Redis backend there does call await self._client.close(), that does not close the connection pool.

cachews needs to ensure that the Redis instance owns the pool. Or, keep a separate pool somewhere and close it.

async def init(self):
        self._client = self._client_class(connection_pool=self._pool_class.from_url(self._address, **self._kwargs))
        await self._client.initialize()
        self.__is_init = True

should become

async def init(self):
        pool = self._pool_class.from_url(self._address, **self._kwargs)
        self._client = self._client_class.from_pool(pool)
        await self._client.initialize()
        self.__is_init = True

This ensures that the created instance takes_ownership of the pool and closes it when itself is closed. The current syntax
assumes that the caller will take care of closing the connection pool.

Please consider errors like this to be the result of incorrect usage in the library actually using redis-py.

[paul-finary]

Hi,

Thanks for the quick reply !

In my case, I'm not dependent on cashews, but I'm wrapping redis.asyncio.Redis in my own class, like so:

class Wrapper:
    """
    Defines a super class of Redis to add new methods and delay the connection after the initialization.
    """

    def __init__(self):
        self.redis = None

    async def connect(self):
        self.redis = Redis.from_pool(
            BlockingConnectionPool.from_url(
                os.getenv("URL"),
                db=0,
                decode_responses=True,
                max_connections=500,
                timeout=5,
            )
        )

    async def disconnect(self):
        await self.redis.aclose(close_connection_pool=True)

    def __getattr__(self, name):
        return getattr(self.redis, name)

The thing I don't understand, is that a ConnectionError is till raised even though:

1. There is _available_connections in the pool
2. There is less _in_use_connections than max_connections, so the pool can create and add new connections

[kristjanvalur]

can you please attach a traceback?

[paul-finary]

Sure:

CancelledError: null
  File "redis/asyncio/connection.py", line 1170, in get_connection
    async with self._condition:
  File "asyncio/locks.py", line 15, in __aenter__
    await self.acquire()
  File "asyncio/locks.py", line 114, in acquire
    await fut
TimeoutError: null
  File "redis/asyncio/connection.py", line 1169, in get_connection
    async with async_timeout(self.timeout):
  File "__init__.py", line 141, in __aexit__
    self._do_exit(exc_type)
  File "__init__.py", line 228, in _do_exit
    raise asyncio.TimeoutError
ConnectionError: No connection available.
  File "app/models/mixins/notifiable_mixin.py", line 66, in _notify_update
    await wrapper.lpush(f"queue:{queue}", payload)
  File "ddtrace/contrib/redis/asyncio_patch.py", line 22, in traced_async_execute_command
    return await _run_redis_command_async(span=span, func=func, args=args, kwargs=kwargs)
  File "ddtrace/contrib/redis/asyncio_patch.py", line 40, in _run_redis_command_async
    result = await func(*args, **kwargs)
  File "redis/asyncio/client.py", line 601, in execute_command
    conn = self.connection or await pool.get_connection(command_name, **options)
  File "redis/asyncio/connection.py", line 1174, in get_connection
    raise ConnectionError("No connection available.") from err

[kristjanvalur]

That is weird. it is timing out trying to acquire the condition variable. That should never happen
Except, this probably occurs at startup and there is a serialization problem here where the condition variable is held too long.
This problem does not have anything to do with the previous one. I'll see if I can make a simple patch.

[iamdbychkov]

Hello @kristjanvalur Excuse me for bothering you, but why we have to close connections if redis instance never goes out of scope? For example if we have a long-running application isn't it better to simply create a pool and then use it for all requests? Does it mean that all open IDLE connection will be there forever and we should clean it all up ourselves if we don't need it at the moment?

Is there any docs on intended use of a async version of redis?

Personally, now I have a troubles with "ERR max number of clients reached" by redis itself, it's a completely different problem but couldn't help myself commenting, as documentation kinda glosses over async version of a client and you have to look everywhere

[kristjanvalur]

Hi @paul-finary , Could you test with the new redis/asyncio/connection.py which is part of the above-mentioned PR?

[kristjanvalur]

Hello @kristjanvalur Excuse me for bothering you, but why we have to close connections if redis instance never goes out of scope? For example if we have a long-running application isn't it better to simply create a pool and then use it for all requests? Does it mean that all open IDLE connection will be there forever and we should clean it all up ourselves if we don't need it at the moment?

Hi there. You don't have to. But if you are having this problem now, when you weren't having it before, it meant that you were losing connection objects and not explicitly closing them. The old version of redis-py would try to clean them up inthe del handlers, but this is really, really bad practice. For one thing, it causes all async jobs to block.

If you are seing this problem now, it means probably that there is some incorrect use of a connection pool, or a connection is not being correctly returned to the pool. What is your use case?

For using a pool object and re-using it, this is the pattern you should be using:

pool = redis.asyncio.ConnectionPool(...)
# make sure we close the pool when we are don
async with contextlib.aclosing(pool):
    for whatever in jobs():
        # make sure we return the connection to the pool after use
        async with redis.asyncio.Redis(connection_pool=pool) as redis:
            redis.do stuff()

[kristjanvalur]

If you forget to return/close connections, then Python will print out a "resource warning" when the connection object gets garbage collected.

[DPham-X]

async Redis client and connection pools now need to be properly closed. The bug is in cachews. While the Redis backend there does call await self._client.close(), that does not close the connection pool.

cachews needs to ensure that the Redis instance owns the pool. Or, keep a separate pool somewhere and close it.

async def init(self):
        self._client = self._client_class(connection_pool=self._pool_class.from_url(self._address, **self._kwargs))
        await self._client.initialize()
        self.__is_init = True

should become

async def init(self):
        pool = self._pool_class.from_url(self._address, **self._kwargs)
        self._client = self._client_class.from_pool(pool)
        await self._client.initialize()
        self.__is_init = True

This ensures that the created instance takes_ownership of the pool and closes it when itself is closed. The current syntax assumes that the caller will take care of closing the connection pool.

Please consider errors like this to be the result of incorrect usage in the library actually using redis-py.

Thanks, I'll follow up with cashews

[paul-finary]

@kristjanvalur Thanks for being so reactive ! I'm currently trying to reproduce the issue as I did not yet find a consistent trigger for the "No connection available" error. I'll keep you posted !

[kristjanvalur]

@kristjanvalur Thanks for being so reactive ! I'm currently trying to reproduce the issue as I did not yet find a consistent trigger for the "No connection available" error. I'll keep you posted !

My hypothesis is this: You create a pool, and immediately start a few hundred requests. However, the current implementation will make the socket connection within the condition variable lock. and so, all the tasks are waiting, on the timeout, while one after the other finishes their connections. A few hundred socket connects can easily take up to 5 seconds, and then finally one of them times out. Maybe you can repro using that, maybe your server was far away or something?

My PR should fix this, now the condition variable is only around pool management, not the socket connection code.