Opened 4 years ago

Closed 3 years ago

#32076 closed New feature (fixed)

Adding async methods to BaseCache

Reported by: Andrew Chen Wang Owned by: Andrew Chen Wang
Component: Core (Cache system) Version: dev
Severity: Normal Keywords: cache
Cc: Andrew Godwin, Carlton Gibson Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by 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.

Change History (16)

comment:1 by Andrew Chen Wang, 4 years ago

Description: modified (diff)
Version: 3.1master

comment:2 by Andrew Chen Wang, 4 years ago

Needs tests: set

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.

comment:3 by Mariusz Felisiak, 4 years ago

Cc: Andrew Godwin added
Needs tests: unset
Resolution: needsinfo
Status: newclosed

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?

comment:4 by Andrew Godwin, 4 years ago

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.

comment:5 by Mariusz Felisiak, 4 years ago

Cc: Carlton Gibson added
Resolution: needsinfo
Status: closednew
Triage Stage: UnreviewedAccepted

Thanks Andrew.

comment:6 by Andrew Chen Wang, 4 years ago

Owner: changed from nobody to Andrew Chen Wang
Status: newassigned

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.

comment:7 by Andrew Chen Wang, 4 years ago

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.

Last edited 4 years ago by Andrew Chen Wang (previous) (diff)

comment:8 by Andrew Chen Wang, 4 years ago

Description: modified (diff)
Summary: Adding async methods to BaseCacheAdding async methods to Base, Dummy, and LocMem cache backends

in reply to:  4 comment:9 by Andrew Chen Wang, 4 years ago

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!

comment:10 by Mariusz Felisiak, 4 years ago

Summary: Adding async methods to Base, Dummy, and LocMem cache backendsAdding async methods to BaseCache

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.

comment:11 by Andrew Godwin, 4 years ago

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.

comment:12 by Andrew Chen Wang, 4 years ago

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?

comment:13 by Carlton Gibson, 4 years ago

Has patch: set

comment:14 by Mariusz Felisiak, 4 years ago

Needs documentation: set
Patch needs improvement: set

comment:15 by Mariusz Felisiak, 3 years ago

Needs documentation: unset
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:16 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 301a85a:

Fixed #32076 -- Added async methods to BaseCache.

This also makes DummyCache async-compatible.

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