Opened 4 days ago

Closed 3 days ago

Last modified 42 hours ago

#36819 closed Cleanup/optimization (wontfix)

Async login does not reuse authenticated user instance

Reported by: Mykhailo Havelia Owned by:
Component: contrib.auth Version: 6.0
Severity: Normal Keywords: async auser alogin cache
Cc: Mykhailo Havelia Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Problem

When using alogin, calling request.auser does not reuse the already authenticated user instance, resulting in redundant authentication requests.

Details

Middleware Setup

class AuthenticationMiddleware(MiddlewareMixin):
    def process_request(self, request):
        ...
        request.user = SimpleLazyObject(lambda: get_user(request))
        request.auser = partial(auser, request)

Sync Login

def login(request, user, backend=None):
    ...
    request.user = user

Async Login

def alogin(request, user, backend=None):
    ...
    request.user = user

Problematic Behavior

After calling alogin(request, user), request.user is set to the authenticated user instance. However, request.auser() does not use this instance and instead performs authentication again, relying on its own _acached_user cache.

Expected Behavior

After alogin, both request.user and request.auser() should reference the same user instance, avoiding duplicate authentication.

Suggested Solution

Set a shared cache. request._cached_user = user, in alogin, so both sync and async accessors use the same instance.

async def alogin(request, user, backend=None):
    ...
    request.user = user
    request._cached_user = user  # <-- suggested addition

async def auser(request):
    if not hasattr(request, "_cached_user"):   # <-- suggested change (_acached_user -> _cached_user) 
        request._cached_user = await auth.aget_user(request)
    return request._cached_user

See related discussion: https://github.com/django/django/pull/16552#discussion_r1122929933

Change History (4)

comment:1 by Natalia Bidart, 3 days ago

Keywords: auser alogin cache added
Resolution: wontfix
Status: newclosed

Hello Mykhailo Havelia, thank you for your ticket and for linking to the PR conversation.

I gotta say, as soon as I read this ticket description, my first gut reaction was twofold:

  1. why a project would mix async and sync in the same flow, and
  2. a shared cache feels like the perfect recipe for a debugging nightmare

Reading Mariusz and Carlton's comment in the PR, seems like we are in agreement. Closing following that rationale, if you feel strongly that this caching is key for a given use case, please share specific details in how a single request would benefit from it.

in reply to:  1 ; comment:2 by Mykhailo Havelia, 2 days ago

Replying to Natalia Bidart:

why a project would mix async and sync in the same flow, and

There is no mixing of sync and async within the same flow. The issue is that after calling alogin and awaiting request.auser(), we end up performing redundant authentication requests (at least against the database).

a shared cache feels like the perfect recipe for a debugging nightmare

It's just one of the options how to solve it. Btw 🙂 that "perfect recipe for a debugging nightmare" already exists in the current codebase. For example:

async def alogin(request, user, backend=None):
    ...
    request.user = user

Here, alogin is an async function that mutates request.user, which is part of the synchronous API.

in reply to:  2 ; comment:3 by Natalia Bidart, 2 days ago

Replying to Mykhailo Havelia:

Replying to Natalia Bidart:

why a project would mix async and sync in the same flow, and

There is no mixing of sync and async within the same flow. The issue is that after calling alogin and awaiting request.auser(), we end up performing redundant authentication requests (at least against the database).

Could you provide exact details and a complete description of this use case? So we could evaluate further. Thank you!

in reply to:  3 comment:4 by Mykhailo Havelia, 42 hours ago

Replying to Natalia Bidart:

I found a related ticket — https://code.djangoproject.com/ticket/36540 — which appears to have been fixed a few months ago 😌

async def auser():
    return user

request.auser = auser

Sorry for taking your time. I'll make sure to check against the current master next time.

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