Memory leak with `redis` backend when `hiredis` is installed

[jamestrousdale]

The problem

Not so much a bug with aiohttp-client-cache per se, but it can be impacted by upstream bugs. There may also be a chance for optimization.

I filed redis/redis-py#2741 against apparent memory leaks from redis-py when hiredis is installed.

As part of the investigation, I realized that even if we provide our own Redis connection instance to aiohttp-client-cache, it still takes the liberty to close the connection on our behalf after the HTTP request session ends.

I think that if the connection object is supplied (as opposed to the redis URL), then aiohttp-client-cache should not close the Redis connection, as the client code may want to continue to use that connection object/associated connection pool.

The problem is not big in one-off scripts, but in a long-running microservice (where we discovered the issue originally), it will definitely lead to OOMs.

Expected behavior

If the user supplies a Redis object in lieu of the URL, don't close connections.

Steps to reproduce the behavior

See redis/redis-py#2741 - the connections created as part of the RedisBackend objects needed for Redis-backed response caching should make it obvious how to reproduce in aiohttp-client-cache

Workarounds

Don't use hiredis when using redis backend

Environment

• aiohttp-client-cache version: 0.8.1
• Python version: 3.11.3
• Platform: Reproduced on both MacOS (arm64) and Linux (amd64)

[JWCook]

I see, thanks for the info!

The connection closing is happening here (initiated by aiohttp.ClientSession.__aexit__():

aiohttp-client-cache/aiohttp_client_cache/session.py

Lines 65 to 68 in 7647800

	async def close(self):
	"""Close both aiohttp connector and any backend connection(s) on contextmanager exit"""
	await super().close()
	await self.cache.close()

I'm not sure what the best way to handle that would be, but here are a couple options:

• Add another keyword arg like backend_autoclose=False
• Leave backend connections open and let the user explicitly close them if needed (will need a bit more testing to see if that would cause problems for any of the other backends)

[jamestrousdale]

Thanks for the response @JWCook

I was wondering if it might make sense to set a flag self._external_connection = True on the RedisCache instance if the user supplies the connection, and if so, chase the close implementation for RedisCache to something like

    async def close(self):
        if self._connection and not self._external_connection:
            await self._connection.close()
            self._connection = None

This seems like a redis-specific issue for this library and if I understand right, the impact would be contained.

This would allow the user to maintain a singleton connection or some similar construct externally, supply it to the aiohttp-client-cache, and not have aiohttp-client-cache close it on them so it can be reused again later.

The case of the user supplying the URL would be left unaffected (in that case, it seems dicey and starting to violate separate concerns for aiohttp-client-cache to take responsibility of maintaining a singleton pool), and when the hiredis bug is fixed eventually, there's nothing to be concerned about anyway.

[jamestrousdale]

FYI seems the latest release of hiredis 2.2.3 has already resolved the observed leak.

I do still think it might be good to allow users the option to manage the connection if they choose by supplying their own connection object which isn't destroyed on teardown.

[JWCook]

Basically I'd like to do whatever is least surprising to users. After considering about it some more, I think auto-closing backend connections probably isn't obvious behavior.

Since I introduced this change fairly recently (0.8.1), I think it might be best to revert it, and leave it up to the user to close those backend connections if they want (same behavior as <=0.8.0).

[JWCook]

I added an autoclose option, which will be available in the next patch release.