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 )
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 , 4 years ago
Description: | modified (diff) |
---|---|
Version: | 3.1 → master |
comment:2 by , 4 years ago
Needs tests: | set |
---|
comment:3 by , 4 years ago
Cc: | added |
---|---|
Needs tests: | unset |
Resolution: | → needsinfo |
Status: | new → closed |
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 , 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 , 4 years ago
Cc: | added |
---|---|
Resolution: | needsinfo |
Status: | closed → new |
Triage Stage: | Unreviewed → Accepted |
Thanks Andrew.
comment:6 by , 4 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
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 , 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.
comment:8 by , 4 years ago
Description: | modified (diff) |
---|---|
Summary: | Adding async methods to BaseCache → Adding async methods to Base, Dummy, and LocMem cache backends |
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:
Let me know if I should just use the argument names themselves.