Opened 3 months ago

Closed 3 months ago

#36455 closed Bug (invalid)

Cache backends delegate async methods to BaseCache implementations instead of using the backend's native support for incr(), get_many(), etc..

Reported by: LaughInJar Owned by:
Component: Core (Cache system) Version: 5.2
Severity: Normal Keywords: aincr adecr
Cc: LaughInJar Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Both the Redis and Memcached backends do not override the async methods of the BaseCache base class. However the base class does not simply wrap all the sync methods using sync_to_async but instead recreate the functionality of methods like incr() or get_many() by using aget() and aset().

For example, the aincr() method in the BaseCache looks like this:

async def aincr(self, key, delta=1, version=None):
    """See incr()."""
    value = await self.aget(key, self._missing_key, version=version)
    if value is self._missing_key:
        raise ValueError("Key '%s' not found" % key)
    new_value = value + delta
    await self.aset(key, new_value, version=version)
    return new_value

However, both Redis and Memcached have native support for incrementing a key the respective Django backends override the BaseCache 's sync methods to use these native functions. But since they do not override the async methods, they fall back to the implementation of BaseCache which uses aget and aset.

This is not only a performance drawback since this requires additional calls, it also means that the calls to aincr are no longer atomic. The docs say that atomicity is not guaranteed but it it will be atomic when the backend supports it. Redis and Memcache support it.

This is relevant when e.g. trying to implement a throttle for API calls.

In order to reproduce, recreate a new virtualenv and install Django.

import asyncio
from django.core.cache.backends.redis import RedisCache

cache = RedisCache("redis://localhost:6379/4", {})

# uncomment this to 'fix' the issue
# from asgiref.sync import sync_to_async
# cache.aincr = sync_to_async(cache.incr)

key = "counter"
cache.add(key, 0, timeout=1)

async def task() -> int:
    return await cache.aincr(key)

async def run():
    tasks = [task() for _ in range(10)]
    return await asyncio.gather(*tasks)

counters = sorted(asyncio.run(run()))

print(counters)
# outputs [1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
# the expected output should be 
# [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]

Although I've focused on aincr() I can see from the code that other methods like ahas_key, aset_many, aget_many, adelete_many, and aget_or_set are similarily emulated using aget and aset. Redis and Memcache have native support for these functions which is utilized when calling the sync methods. The expectation would be that the async methods would work in the same manner as the sync methods.

Change History (2)

comment:1 by LaughInJar, 3 months ago

I thought I've read the whole documentation carefully beforehand, but just after hitting send I revisted that section https://docs.djangoproject.com/en/5.2/topics/cache/#id16

I think it covers the things I've discussed above even when it only becomes clear after one did the deep dive into the codebase on why the likes of aincr do not work as expected. Maybe this ticket at least saves somebody else time doing research.

comment:2 by Sarah Boyce, 3 months ago

Resolution: invalid
Status: newclosed

Thank you
Note there is a ticket for adding native async support to the redis backend: https://code.djangoproject.com/ticket/33573

Note: See TracTickets for help on using tickets.
Back to Top