Adding async methods to BaseCache

[Andrew Chen Wang]

I've recently created a new package for Redis and Django integration at ​django-async-redis. I'd like to add the missing methods, e.g. get_async or set_async to the BaseCache so people can get type hints or autocompletion suggestions when using django.core.cache.cache with async.

Additionally, in order to be compatible with the async methods, there would need to be a new async method for closing cache connections/pools at ​django.core.cache.backends.base. I believe the only good solution to that would be:

async def close_caches_async(**kwargs):
    # Some caches -- python-memcached in particular -- need to do a cleanup at the
    # end of a request cycle. If not implemented in a particular backend
    # cache.close is a no-op
    for cache in caches.all():
        await cache.close_async()

Please let me know if that is out of scope though. I also believe implementing async methods for the current backends is also out of scope of this ticket (and my time :P). Edit: on second thought, I will add the methods to DummyCache as well.

For reference, the Google Group Discussion: ​here and ​here

Edit: I've decided to add the three main cache backends -- BaseCache, DummyCache, LocMemCache -- with the necessary test cases which can be used for all future async test cases for stuff like DBCache. Please read the latest commit for which test cases are notable exceptions to the main BaseCacheTests mixin.

[Andrew Chen Wang]

Please let me know if that is out of scope though.

I think it's out of scope since Django signals are still synchronous I believe...

I've also added test for the DummyCache by when inheriting BaseCache, I just set up the methods like so:

async def set_async(self, *args, **kwargs):
    return self.set(*args, **kwargs)

Let me know if I should just use the argument names themselves.

[Mariusz Felisiak]

It looks that your proposition doesn't match with accepted ​DEP-9:

Default implementations of these that just call the existing API via sync_to_async will be provided in BaseCache.

Moreover, I'm not sure if we want to release any partial solution without a real async support in caching. DEP 9 suggests that a separate DEP is required:

Some features, like templating and cache backends, will need their own separate DEPs and research to be fully async. This DEP mostly focuses on the HTTP-middleware-view flow and the ORM.

Initially closing as needsinfo. Andrew, Can I ask for your opinion?

[Andrew Godwin]

My interpretation of DEP-9 is that we need to add default _async variant methods to BaseCache that pass through to the synchronous versions - this seems to be what the ticket is proposing, on its surface.

The cache closing problem is a bit larger, though - if we need to do more work than just slapping async methods into the system that use sync_to_async to make it all work properly, then I suspect we're going to have to do this in a bit more of a formal process than just a single ticket/PR.

[Mariusz Felisiak]

Thanks Andrew.

[Andrew Chen Wang]

Just what I have for now: ​PR #13508. It implements the BaseCache and DummyCache with test cases. I did not add my commit for LocMem since I didn't finish writing the tests... because I was basically copying 2,000 lines of test case code and sticking in an await or async every once in awhile. If you think I should add it, then please let me know.

Regarding documentation... I don't know... I'm not very good at it. I could possibly just say "to use this asynchronously, you can await the method with a suffix of _async". Besides that, I don't want to ruin Django awesome rep for docs.

[Andrew Chen Wang]

The cache closing problem is a bit larger, though

I wanted to put this in a separate comment because, while writing my PR comment, I realized the issue of adding async cache to Django might be larger but this ticket is important in order to start porting the rest of Django to async. Lots of things like the SessionCache backend and generally closing the cache using Signals requires adding a lot of code; without those async methods i.e. this ticket, we can't actually implement the code that uses the cache.

To me, if DEP 9 is to implement Django templates, middleware, signals, etc. as asynchronous, the base layer of the cake needs to be set first: that means getting the cache and database access ready beforehand.

Edit: So in essence, I think this ticket can at least cover getting the BaseCache ready for future usage of async. Perhaps instead of raising a NotImplementedError like what felixxm said, then I can change that for sync_to_async usage so that when a Django version upgrade happens, then those using something like FileBasedCache are not immediately given huge errors. I've got LocMem down in a separate commit, but I definitely won't be handling all Django cache backends.

[Andrew Chen Wang]

Replying to Andrew Godwin:

Hey Andrew, do you mind checking out if the LocMem and Base Cache is what you thought about when implementing async caching? I'd also like to know if there's a way to be more DRY with the docstrings.

Sorry if the commit is HUGE. The majority is just copying the test cases and making use of the async versions. The LocMem backend changes just use asyncio.Lock(). Thanks guys!

[Mariusz Felisiak]

Please don't change a scope of accepted tickets. This ticket is only about BaseCache:

My interpretation of DEP-9 is that we need to add default _async variant methods to BaseCache that pass through to the synchronous versions - this seems to be what the ticket is proposing, on its surface.

Any attempt to implement async cache backends should be preceded by a discussion (e.g. ​Async Cache, and a helper for porting sync code and probably an accepted DEP.

[Andrew Godwin]

I took a quick look through the PR and the most notable thing is that you've fully redeclared all the methods on BaseCache - there's no need to do that, since sync_to_async will automatically handle default arguments and docstrings and the like!

Instead, you should be able to do:

add_async = sync_to_async(add, thread_sensitive=True)

The only way I can think this might fall down is if those methods don't end up bound to "self" properly, but if so that's something we should just fix once in asgiref if we can.

And I agree with felixxm - let's keep this to just BaseCache purely for now, and just add a few tests to make sure that, as planned, it automatically follows through onto the other backends in a safe enough way.

[Andrew Chen Wang]

Re-implemented in a new PR without all the extras (I'm sorry about that): ​https://github.com/django/django/pull/13547 I took into account what Andrew Godwin said, but I'm a little unclear about what I should actually be changing. Are you saying change all the async functions to use sync_to_async or how I've currently done it?

[Mariusz Felisiak <felisiak.mariusz@…>]

In 301a85a:

Fixed #32076 -- Added async methods to BaseCache.

This also makes DummyCache async-compatible.