Opened 4 years ago

Last modified 3 years ago

#32076 closed New feature

Adding async methods to Base, Dummy, and LocMem cache backends — at Version 8

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 (8)

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
Note: See TracTickets for help on using tickets.
Back to Top