Skip to content

Conversation

Andrew-Chen-Wang
Copy link
Contributor

@Andrew-Chen-Wang Andrew-Chen-Wang commented Oct 7, 2020

ticket-32076

  • Based on DEP 0009 and the implementation found here
  • Copied all doc strings from the synchronous methods
  • Adds async cache methods to BaseCache and DummyCache.

Issues I currently see are:

  1. The request finished signal being unable to properly close all cache connections (more details in original ticket). This can be found in django.core.cache (init file).
  2. This has been implemented as per DEP 0009 Lots of times in several areas in Django, the cache is called by doing if "string" in cache. The async method cannot handle the magic method __contains__ unless you use sync_to_async. I dislike context switching, so I would rather not touch that, but I can see why people would want to "accidentally" use it, even if in the documentation we mention it will not be performant and people should strive to use await cache.has_key("string") instead.
  3. Current session backend is not handling the async methods. @felixxm Made a good point about being a partial solution and why BaseCache may not be good enough for this implementation; I'm currently writing a session backend for my websocket implementation found here (I'll be moving the code to a separate repository soon to demonstrate the functionality). The issue I immediately ran into was logging in since I wanted to see how I can validate the session for that custom websocket script. If we went with what @felixxm said, then the async methods in BaseCache would look like:
async def get_async(value):
    return sync_to_async(self.get(value))

The issue I see with that is if a library accidentally doesn't implement the get_async() method, then the error will say that the get method is not created when it should really say the dev needs to create the get_async method.
4. I'm not going to have time to write documentation (Edit rephrased: I'm terrible at documentation and heavily discouraged).
5. I've added LocMem to this PR. The only change is that we're adding asyncio.Lock for async context. I think I can rework this to take advantage of the RunTimeError so we know which context we're in. That means we don't need to create two different locks. If you'd like, I can add my setup for LocMem backend. I'm not too sure about the tests though. As I was copying and pasting all those test cases, my computer started burning up...

* Based on DEP 0009 and the implementation found [here](https://github.com/Andrew-Chen-Wang/django-async-redis)
* Copied all doc strings from the synchronous methods
@Andrew-Chen-Wang
Copy link
Contributor Author

Andrew-Chen-Wang commented Oct 8, 2020

So as I'm implementing LocMem (I think I want to add this in a different ticket), I'm writing the async test cases right next to their synchronous versions. An issue arising is if the currently tested backend doesn't have the _async ending, which would support the point that we should use the sync_to_async by default. So I'll be adding that.

Let me know if LocMem should be implemented in this ticket or in a separate ticket btw. Test cases are mostly passing... just a couple of sync_to_async methods failing.

Edit: I finished writing the LocMem tests. Default use sync_to_async

@Andrew-Chen-Wang Andrew-Chen-Wang changed the title Fixes #32076 -- Add async cache methods to BaseCache Fixes #32076 -- Add async cache methods to Base, Dummy, and LocMem Cache Oct 8, 2020
* Per DEP 9, thanks to @felixxm, the async methods of the cache default to running the synchronous version.
* Accidentally closed the cache in decr_version_async

Signed-off-by: Andrew-Chen-Wang <[email protected]>
* Locks implement asyncio.Lock if in an async context
* All BaseTest cases for cache were copied from their synchronous versions
* Added async LocMem specific test cases

Extra Notes:

* test_cache_write_unpicklable_object_async requires the Middleware mixin to be asynchronous. Thus I have left a TODO in L1825
* test_per_thread_async creates a new event loop for each new thread in order to test with async locks

Signed-off-by: Andrew-Chen-Wang <[email protected]>
* Some test cases needed to be overridden in the Memcached specific test cases. The async versions of those sync overriding test cases are now implemented

Signed-off-by: Andrew-Chen-Wang <[email protected]>
@Andrew-Chen-Wang
Copy link
Contributor Author

Andrew-Chen-Wang commented Oct 8, 2020

Oh yeah... need to support python 3.6/7.... So the problem was AsyncMock was added in Python 3.8... is there any AsyncMock implemented in Django's tests is the question...

* Had to add AsyncMock support for Python 3.6-7 support in test_get_or_set_racing_async
* Also linted with flake8 for code that wasn't linted before?

Signed-off-by: Andrew-Chen-Wang <[email protected]>
@Andrew-Chen-Wang
Copy link
Contributor Author

Andrew-Chen-Wang commented Oct 8, 2020

Sigh... one Python version, one memcached library, one async test case with no sync version that was overriding... I'm not even sure how to fix this one...

@felixxm
Copy link
Member

felixxm commented Oct 9, 2020

Please don't change a scope of accepted tickets, see comment.

@Andrew-Chen-Wang
Copy link
Contributor Author

New PR instead of reverting

@Andrew-Chen-Wang Andrew-Chen-Wang deleted the ticket_32076 branch July 6, 2023 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants