[Django] #31920: ASGI/ASYNC SessionMiddleware - SynchronousOnlyOperation exception if request.user is not unwrapped in sync code

[Django]

#31920: ASGI/ASYNC SessionMiddleware - SynchronousOnlyOperation exception if
request.user is not unwrapped in sync code
------------------------------------------------+------------------------
Reporter: Michael Galler | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.sessions | Version: 3.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
{{{
class AuthenticationMiddleware(MiddlewareMixin):
def process_request(self, request):
request.user = SimpleLazyObject(lambda: get_user(request))

}}}

AuthenticationMiddleware wraps the request.user in a SimpleLazyObject, so
the Database Query is called lazy

If i start now a project via django-admin and I add a user and log in and
request the following view:

{{{
from asgiref.sync import sync_to_async
from django.core.exceptions import SynchronousOnlyOperation
from django.http import HttpResponse

@sync_to_async
def get_user_from_request(request):
return request.user if bool(request.user) else None

async def async_test(request):
# CASE 1
try:
print(request.user.is_authenticated)
except SynchronousOnlyOperation:
print('error 1')

# CASE 2
try:
user = await sync_to_async(request.user)
print(user)
except SynchronousOnlyOperation:
print('error 2')

# CASE 3
try:
user = await sync_to_async(request.user)()
print(user)
except SynchronousOnlyOperation:
print('error 3')

# CASE 4
try:
user = await sync_to_async(bool)(request.user)
print(request.user)
except SynchronousOnlyOperation:
print('error 4')

# CASE 5
try:
user = await get_user_from_request(request)
print(user)
except SynchronousOnlyOperation:
print('error 5')

return HttpResponse('Hello, async world!')

}}}

Only case 4 and 5 works, i find it a little awkward that i have to do one
of this cases.

--
Ticket URL: <https://code.djangoproject.com/ticket/31920>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#31920: ASGI/ASYNC SessionMiddleware - SynchronousOnlyOperation exception if
request.user is not unwrapped in sync code

----------------------------------+------------------------------------

Reporter: Michael Galler | Owner: nobody

Type: New feature | Status: new
Component: contrib.sessions | Version: 3.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

----------------------------------+------------------------------------
Changes (by Carlton Gibson):

* cc: Andrew Godwin (added)
* type: Cleanup/optimization => New feature
* stage: Unreviewed => Accepted

Comment:

HI Michael, I’m going to accept this as a New Feature.

I accept totally that you just want to do `request.user`, and that’s
clearly where we need to go, but I don’t think that’s entailed by the view
and middleware support in 3.1, so this is something we need to think about
and address.

(If you want to help there, that would be cool.)

--
Ticket URL: <https://code.djangoproject.com/ticket/31920#comment:1>

[Django]

#31920: ASGI/ASYNC SessionMiddleware - SynchronousOnlyOperation exception if
request.user is not unwrapped in sync code

----------------------------------+------------------------------------

Reporter: Michael Galler | Owner: nobody

Type: New feature | Status: new
Component: contrib.sessions | Version: 3.1

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

----------------------------------+------------------------------------

Comment (by Andrew Godwin):

So the problem here is that attribute access is always synchronous, so
lazy loading simply isn't possible. `SynchronousOnlyOperation` was added
to at least defend against this being done by accident, but if we want
`request.user` to simply work during async views, it will need to be
greedily loaded at middleware execution time.

This is tricky, as the middleware doesn't know if the view below it is
sync or async. There's a variety of solutions to this - a setting to
always greedily load it, a nice-ish async-compatible function on
user/SimpleLazyObject that loads it, trying to escape to the async loop
using greenlets - but I'm not sure which one I prefer right now.

--
Ticket URL: <https://code.djangoproject.com/ticket/31920#comment:2>

[Django]

#31920: ASGI/ASYNC SessionMiddleware - SynchronousOnlyOperation exception if
request.user is not unwrapped in sync code

----------------------------------+------------------------------------

Reporter: Michael Galler | Owner: nobody

Type: New feature | Status: new
Component: contrib.sessions | Version: 3.1

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

----------------------------------+------------------------------------

Comment (by Michael Galler):

would not something like the following be easier?

{{{
class AuthenticationMiddleware(MiddlewareMixin):
def process_view(self, request, view_func, args, kwargs):
hasattr(request.user, 'foobar')

}}}

This unpacks the user after all process_requests calls and before any
views are called, also there are no conflicts with other middlewares like
RemoteUserMiddleware.

--
Ticket URL: <https://code.djangoproject.com/ticket/31920#comment:3>

[Django]

#31920: ASGI/ASYNC SessionMiddleware - SynchronousOnlyOperation exception if
request.user is not unwrapped in sync code

----------------------------------+------------------------------------

Reporter: Michael Galler | Owner: nobody

Type: New feature | Status: new
Component: contrib.sessions | Version: 3.1

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

----------------------------------+------------------------------------

Comment (by Andrew Godwin):

Right, that's just removing the lazy-loading aspect of it for every
request ("greedily loading it"). We can't ship that in default Django as
it would be a performance regression from now, essentially removing the
lazy loading part permanently. This is why I suggested we maybe do this
with a setting.

However, it's a useful workaround if you want to write your own
middleware.

Replying to [comment:3 Michael Galler]:

> would not something like the following be easier?
>
>
> {{{
> class AuthenticationMiddleware(MiddlewareMixin):
> def process_view(self, request, view_func, args, kwargs):
> hasattr(request.user, 'foobar')
>
> }}}
>
> This unpacks the user after all process_requests calls and before any
views are called, also there are no conflicts with other middlewares like
RemoteUserMiddleware.

--
Ticket URL: <https://code.djangoproject.com/ticket/31920#comment:4>

[Django]

#31920: ASGI/ASYNC SessionMiddleware - SynchronousOnlyOperation exception if
request.user is not unwrapped in sync code

----------------------------------+------------------------------------

Reporter: Michael Galler | Owner: nobody

Type: New feature | Status: new
Component: contrib.sessions | Version: 3.1

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

----------------------------------+------------------------------------

Comment (by Michael Galler):

Replying to [comment:4 Andrew Godwin]:

> Right, that's just removing the lazy-loading aspect of it for every
request ("greedily loading it"). We can't ship that in default Django as
it would be a performance regression from now, essentially removing the
lazy loading part permanently. This is why I suggested we maybe do this
with a setting.
>
> However, it's a useful workaround if you want to write your own
middleware.

Wouldn't it make sense to have a decorator that automatically executes the
function synchronously thread sensitive when it is executed in a async
context?

What i came up with after some time is the following:

{{{
async def run_in_sync(func, current_executor, *args, **kwargs):
if current_executor:
AsyncToSync.executors.current = current_executor
return await sync_to_async(func, thread_sensitive=True)(*args,
**kwargs)

def ensure_sync_environment(message):
def decorator(func):
@functools.wraps(func)
def inner(*args, **kwargs):
try:
loop = asyncio.get_running_loop()
except RuntimeError:
return func(*args, **kwargs)
else:
current_executor = getattr(AsyncToSync.executors,
"current", None)
with ThreadPoolExecutor(max_workers=1) as executor:
return executor.submit(asyncio.run, run_in_sync(func,
current_executor, *args, **kwargs)).result()

return inner

# If the message is actually a function, then be a no-arguments
decorator.
if callable(message):
func = message
message = 'You cannot call this from an async context - use a
thread or sync_to_async.'
return decorator(func)
else:
return decorator
}}}
If I now decrorate the function get_user with it

{{{

@ensure_sync_environment
def get_user(request):
if not hasattr(request, '_cached_user'):
request._cached_user = auth.get_user(request)
return request._cached_user

}}}
The problem is gone.
This works with asgi and wsgi and does not break any thread sensitivity,
even the database connections are correctly cleared.

--
Ticket URL: <https://code.djangoproject.com/ticket/31920#comment:5>

[Django]

#31920: ASGI/ASYNC SessionMiddleware - SynchronousOnlyOperation exception if
request.user is not unwrapped in sync code

----------------------------------+------------------------------------

Reporter: Michael Galler | Owner: nobody

Type: New feature | Status: new
Component: contrib.sessions | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

----------------------------------+------------------------------------
Changes (by Carlton Gibson):

* cc: Carlton Gibson (added)
* keywords: => async

--
Ticket URL: <https://code.djangoproject.com/ticket/31920#comment:6>

[Django]

#31920: ASGI/ASYNC SessionMiddleware - SynchronousOnlyOperation exception if
request.user is not unwrapped in sync code

----------------------------------+------------------------------------
Reporter: Michael Galler | Owner: lirontb
Type: New feature | Status: assigned
Component: contrib.sessions | Version: 3.1

Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

----------------------------------+------------------------------------
Changes (by lirontb):

* owner: nobody => lirontb
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/31920#comment:9>

[Django]

#31920: ASGI/ASYNC SessionMiddleware - SynchronousOnlyOperation exception if
request.user is not unwrapped in sync code

----------------------------------+------------------------------------
Reporter: Michael Galler | Owner: lirontb
Type: New feature | Status: assigned

Component: contrib.sessions | Version: 3.1

Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

----------------------------------+------------------------------------

Comment (by Andrew Godwin):

Chatted with lirontb about this at the DCUS2022 sprints, and we believe we
can just implement the `__await__` method on SimpleLazyObject, roughly
like this:

{{{
class X:
def __await__(self):
if isinstance(self.wrapped, coroutine):
async def inner():
result = await coroutine
self.wrapped = result
return result
return inner().__await__()
else:
async def result():
return self.wrapped
return result().__await__()
}}}

That would mean we could simply `await request.user` (or any other
SimpleLazyObject) and get the value out. If this works, we could even
consider this for the related object descriptors for foreign keys...!

--
Ticket URL: <https://code.djangoproject.com/ticket/31920#comment:10>

[Django]

#31920: ASGI/ASYNC SessionMiddleware - SynchronousOnlyOperation exception if
request.user is not unwrapped in sync code

----------------------------------+------------------------------------
Reporter: Michael Galler | Owner: lirontb
Type: New feature | Status: assigned

Component: contrib.sessions | Version: 3.1

Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

----------------------------------+------------------------------------
Changes (by lirontb):

* has_patch: 0 => 1

Comment:

[https://github.com/django/django/pull/16219 PR]

--
Ticket URL: <https://code.djangoproject.com/ticket/31920#comment:11>

[Django]

#31920: ASGI/ASYNC SessionMiddleware - SynchronousOnlyOperation exception if
request.user is not unwrapped in sync code

----------------------------------+------------------------------------
Reporter: Michael Galler | Owner: lirontb
Type: New feature | Status: assigned

Component: contrib.sessions | Version: 3.1

Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

----------------------------------+------------------------------------
Changes (by Mariusz Felisiak):

* needs_better_patch: 0 => 1
* needs_docs: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/31920#comment:12>

[Django]

#31920: ASGI/ASYNC SessionMiddleware - SynchronousOnlyOperation exception if
request.user is not unwrapped in sync code

----------------------------------+------------------------------------
Reporter: Michael Galler | Owner: lirontb
Type: New feature | Status: assigned

Component: contrib.sessions | Version: 3.1

Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

----------------------------------+------------------------------------

Comment (by Adam Johnson):

I don’t like the suggestion of adding `SimpleLazyObject.__await__`. It’s
too “magical” and it makes the type of `request.user` complex.

Currently django-stubs types `request.user` as `User | AnonymousUser`
(with a documented technique to use just `User`
[https://github.com/typeddjango/django-stubs#how-can-i-create-a
-httprequest-thats-guaranteed-to-have-an-authenticated-user for logged in
requests]). If we wanted to support the `__await__` method in type hints,
then the type would become `User | AnonymousUser | Awaitable[[], User |
AnonymousUser]`. This would require [https://adamj.eu/tech/2021/05/17
/python-type-hints-how-to-narrow-types-with-isinstance-assert-literal/
type narrowing] everywhere `request.user` is used... so, we probably
wouldn’t want to do that in django-stubs, but that would make it harder to
have a correctly type-checked async Django app.

I propose instead making the middleware add a coroutine `request.auser()`
that fetches from `request.user`, used like:

{{{
user = await request.auser()
if user.is_authenticated:
...
}}}

Yes, it’s a little more code, but it follows Django’s other async API’s
and it looks like a normal coroutine call.

--
Ticket URL: <https://code.djangoproject.com/ticket/31920#comment:13>

[Django]

#31920: ASGI/ASYNC SessionMiddleware - SynchronousOnlyOperation exception if
request.user is not unwrapped in sync code

----------------------------------+------------------------------------
Reporter: Michael Galler | Owner: lirontb
Type: New feature | Status: assigned

Component: contrib.sessions | Version: 3.1

Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

----------------------------------+------------------------------------
Changes (by Jon Janzen):

* cc: Jon Janzen (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/31920#comment:14>

[Django]

#31920: ASGI/ASYNC SessionMiddleware - SynchronousOnlyOperation exception if
request.user is not unwrapped in sync code

----------------------------------+------------------------------------
Reporter: Michael Galler | Owner: lirontb
Type: New feature | Status: assigned

Component: contrib.sessions | Version: 3.1

Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

----------------------------------+------------------------------------

Comment (by Carlton Gibson):

I think Adam is likely right here. We should likely have a separate
`auser()`, similarly to elsewhere.

> ''To await, or not await, 'tis the question''

--
Ticket URL: <https://code.djangoproject.com/ticket/31920#comment:15>

[Django]

#31920: ASGI/ASYNC SessionMiddleware - SynchronousOnlyOperation exception if
request.user is not unwrapped in sync code

----------------------------------+------------------------------------
Reporter: Michael Galler | Owner: lirontb
Type: New feature | Status: assigned

Component: contrib.sessions | Version: 3.1

Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

----------------------------------+------------------------------------

Comment (by Jon Janzen):

Should `auser` cache the results of the DB query? AIUI that's how
`SimpleLazyObject` works right now so it should probably be conistent.

For myself, I know there are certain codepaths in my personal installation
that read the equivalent of `request.user` several times during a request.

If it doesn't need to be cached, I see this as a fairly trivial and I'd be
happy to do it. But I'll wait for guidance on my question before moving
forward.

Also, while I'm commenting here: I posted about a larger issue about
asyncifying the auth app overall which touches on this ticket if anyone is
interested in that: https://forum.djangoproject.com/t/asyncifying-django-
contrib-auth-and-signals-and-maybe-sessions/18770

--
Ticket URL: <https://code.djangoproject.com/ticket/31920#comment:16>

[Django]

#31920: ASGI/ASYNC SessionMiddleware - SynchronousOnlyOperation exception if
request.user is not unwrapped in sync code

----------------------------------+--------------------------------------
Reporter: Michael Galler | Owner: Jon Janzen

Type: New feature | Status: assigned

Component: contrib.sessions | Version: 3.1

Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

----------------------------------+--------------------------------------
Changes (by Jon Janzen):

* owner: lirontb => Jon Janzen

Comment:

I went ahead and created a PR with a very simple cache:

[https://github.com/django/django/pull/16552 PR]

--
Ticket URL: <https://code.djangoproject.com/ticket/31920#comment:17>

[Django]

#31920: ASGI/ASYNC SessionMiddleware - SynchronousOnlyOperation exception if
request.user is not unwrapped in sync code

----------------------------------+--------------------------------------
Reporter: Michael Galler | Owner: Jon Janzen
Type: New feature | Status: assigned

Component: contrib.sessions | Version: 3.1

Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1

Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

----------------------------------+--------------------------------------
Changes (by Carlton Gibson):

* needs_tests: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/31920#comment:18>

[Django]

#31920: ASGI/ASYNC SessionMiddleware - SynchronousOnlyOperation exception if
request.user is not unwrapped in sync code

----------------------------------+--------------------------------------
Reporter: Michael Galler | Owner: Jon Janzen
Type: New feature | Status: assigned

Component: contrib.sessions | Version: 3.1

Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0

----------------------------------+--------------------------------------
Changes (by Carlton Gibson):

* cc: Adam Johnson (added)

Comment:

Just to follow-up, there was a [https://github.com/python/mypy/issues/9837
mypy proposal to allow `typing.overload` to work with `T | Awaitable[T]`
type examples], but that was closed as `wontfix`.

> No, this isn't something mypy is going to do. Separate methods … is an
easy solution.

So, I take it that the `auser()` approach is going with the wind.

Current PR is looking OK. I just want to investigate if/how sharing the
cache between `.user` and `.auser()` might work — but any other reviews or
input welcomed.

--
Ticket URL: <https://code.djangoproject.com/ticket/31920#comment:19>

[Django]

#31920: ASGI/ASYNC SessionMiddleware - SynchronousOnlyOperation exception if
request.user is not unwrapped in sync code

----------------------------------+--------------------------------------
Reporter: Michael Galler | Owner: Jon Janzen
Type: New feature | Status: assigned

Component: contrib.sessions | Version: 3.1

Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

----------------------------------+--------------------------------------
Changes (by Jon Janzen):

* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
* needs_docs: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/31920#comment:20>

[Django]

#31920: ASGI/ASYNC SessionMiddleware - SynchronousOnlyOperation exception if
request.user is not unwrapped in sync code

-------------------------------------+-------------------------------------

Reporter: Michael Galler | Owner: Jon
| Janzen
Type: New feature | Status: assigned

Component: contrib.sessions | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/31920#comment:21>

[Django]

#31920: ASGI/ASYNC SessionMiddleware - SynchronousOnlyOperation exception if
request.user is not unwrapped in sync code

-------------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Jon
| Janzen

Type: New feature | Status: closed
Component: contrib.sessions | Version: 3.1
Severity: Normal | Resolution: fixed

Keywords: async | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):

* status: assigned => closed
* resolution: => fixed

Comment:

In [changeset:"e846c5e7246a0ffbe5dcf07a2b6c7c2a47537eb3" e846c5e7]:
{{{
#!CommitTicketReference repository=""
revision="e846c5e7246a0ffbe5dcf07a2b6c7c2a47537eb3"
Fixed #31920 -- Made AuthenticationMiddleware add request.auser().
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/31920#comment:22>