Fixed #34901 -- Added async-compatible interface to session engines. #17372
Conversation
bigfootjon
force-pushed
the
ticket_34901
branch
from
dbaf159
to
481fc55
Compare
|
The tests are in |
bigfootjon
force-pushed
the
ticket_34901
branch
from
481fc55
to
251d566
Compare
|
Thanks @timgraham ! I don't know how I missed those. Tests added. |
There was a problem hiding this comment.
quick glance looks great; not sure if ready for review, but thanks for adding my horrible hybrid mess @bigfootjon!
Yep ready for review, thanks for taking a look! |
bigfootjon
force-pushed
the
ticket_34901
branch
from
251d566
to
391ba40
Compare
|
Thanks for the feedback @felixxm! I've made the requested edits |
bigfootjon
force-pushed
the
ticket_34901
branch
from
391ba40
to
e0bb4eb
Compare
|
@felixxm would you mind taking another look when you get a chance? |
|
Rebased & pushed small edits. |
|
|
||
| @classmethod | ||
| async def aclear_expired(cls): | ||
| pass |
There was a problem hiding this comment.
Don't you think that we should do the same for file-based backend to avoid context switching?
There was a problem hiding this comment.
@bigfootjon What do you think? ☝️
There was a problem hiding this comment.
Is there an async API for files? My understanding is that Python does not expose an async file io model. IIRC somewhere in Django (ASGI support for static files?) that has a context switch due to this same limitation.
There was a problem hiding this comment.
There is aiofiles https://pypi.org/project/aiofiles/
There was a problem hiding this comment.
I figured this wasn’t the right vehicle to take on a new dependency, I think aiofiles would be a larger conversation
There was a problem hiding this comment.
I didn't want to suggest that :) I think it's worth adding for consistency and to avoid unnecessary sync_to_async() wrapping.
There was a problem hiding this comment.
I'm not sure aiofiles is actually what we really want, it just uses a threadpool to do the actual I/O:
aiofiles helps with this by introducing asynchronous versions of files that support delegating operations to a separate thread pool.
(on the main description: https://pypi.org/project/aiofiles/)
This is basically what sync_to_async does so this doesn't seem like it cuts out the context switch :/
There was a problem hiding this comment.
It seems I keep confusing you, sorry for that. I don't want to anything more with the file-base engine. I think we should keep the current versions of async methods (that call sync methods) to avoid sync_to_async() wrapping that is in the base implementation.
There was a problem hiding this comment.
Ooooh I see what you mean now. Sorry for being dense! I agree!
|
@bigfootjon Thanks 👍 I've started working on this patch, please push changes (if any) in a separate commits. |
Thanks for helping clean it up! I’m traveling right now and won’t be able to contribute for the next 2 weeks, just fyi |
felixxm
force-pushed
the
ticket_34901
branch
3 times, most recently
from
248259c
to
61935fb
Compare
Unfortunately, I only have 2 weeks left 😅 |
|
I think it's ready 🎉 @bigfootjon Thanks 👍 |
I just replied to one of your inline comments above about using async file io, but other than that I agree! Thanks for pushing this over the line |
felixxm
changed the title
Thanks Andrew-Chen-Wang for the initial implementation which was posted to the Django forum thread about asyncifying contrib modules.
Ticket: https://code.djangoproject.com/ticket/34901
Forum discussions: https://forum.djangoproject.com/t/asyncifying-django-contrib-auth-and-signals-and-maybe-sessions/18770
This adds an async interface and implementation to
django.contrib.sessions. This is based on the implementation from Andrew-Chen-Wang which was posted to the Django forum thread about asyncifying contrib modules.TODO:Tests: I couldn't find any tests of the existing sessions system (besides the tests I updated to be natively async). Where should the test cases for this go?