Re: [Django] #33716: async get_response can be a regular function too

[Django]

#33716: async get_response can be a regular function too
---------------------------------+------------------------------------
Reporter: abetkin | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 4.0
Severity: Release blocker | 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 abetkin):

* severity: Normal => Release blocker

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

[Django]

#33716: async get_response can be a regular function too
-------------------------------+------------------------------------
Reporter: abetkin | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 4.0

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 abetkin):

* severity: Release blocker => Normal

--
Ticket URL: <https://code.djangoproject.com/ticket/33716#comment:8>

[Django]

#33716: async get_response can be a regular function too
-------------------------------+------------------------------------
Reporter: abetkin | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 4.0
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
-------------------------------+------------------------------------

Description changed by abetkin:

Old description:

> Here is what we have in MiddlewareMixin:
>
> {{{
> def _async_check(self):
> """
> If get_response is a coroutine function, turns us into async mode
> so
> a thread is not consumed during a whole request.
> """
> if asyncio.iscoroutinefunction(self.get_response):
> # Mark the class as async-capable, but do the actual switch
> # inside __call__ to avoid swapping out dunder methods
> self._is_coroutine = asyncio.coroutines._is_coroutine
> else:
> self._is_coroutine = None
> }}}
>
> This checks if the next middleware is a coroutine, and if not fallbacks
> to sync mode. However, we already mark all async-capable middleware as
> such, why the redundancy?
>
> A common usecase that is currently not supported:
>

> {{{
> def MyMiddleware(get_response):
>
> def middleware(request):
> # Do some stuff with request that does not involve I/O
> request.vip_user = True
> return get_response(request)
>
> return middleware
>
> MyMiddleware.async_capable=True
> }}}
>
> middleware(request) will return the response in sync case and a coroutine
> in the async case, despite being a regular function (because get_response
> is a coroutine function in the latter case).
>
> So I propose to remove the redundant _async_check
>
> Github project to see the error: https://github.com/pwtail/django_bug

New description:

Here is what we have in MiddlewareMixin:

{{{
def _async_check(self):
"""
If get_response is a coroutine function, turns us into async mode
so
a thread is not consumed during a whole request.
"""
if asyncio.iscoroutinefunction(self.get_response):
# Mark the class as async-capable, but do the actual switch
# inside __call__ to avoid swapping out dunder methods
self._is_coroutine = asyncio.coroutines._is_coroutine
else:
self._is_coroutine = None
}}}

This checks if the next middleware is a coroutine, and if not fallbacks to
sync mode. However, I think this check is redundant: if the middleware is
async-capable, and we have an ASGI request, what else we need ti check?

The downside of _async_check is that this common usecase is not supported:

{{{
def MyMiddleware(get_response):

def middleware(request):
# Do some stuff with request that does not involve I/O
request.vip_user = True
return get_response(request)

return middleware

MyMiddleware.async_capable=True
}}}

middleware(request) will return the response in sync case and a coroutine
in the async case, despite being a regular function (because get_response
is a coroutine function in the latter case).

Here is a patch that I use that explains a possible way to fix it:

{{{
def call_mw(mw, request, _call_mw=MiddlewareMixin.__call__):
if isinstance(request, ASGIRequest) and mw.async_capable:
return mw.__acall__(request)
return _call_mw(mw, request)

MiddlewareMixin.__call__ = call_mw
}}}

Github project that shows the error: https://github.com/pwtail/django_bug

--

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

[Django]

Old description:

> to sync mode. However, I think this check is redundant: if the middleware

> MiddlewareMixin.__call__ = call_mw
> }}}
>

New description:

sync mode. However, I think this is redundant: if the middleware is async-

capable, and we have an ASGI request, what else we need ti check?

The downside of _async_check is that this common usecase is not supported:

{{{
def MyMiddleware(get_response):

def middleware(request):
# Do some stuff with request that does not involve I/O
request.vip_user = True
return get_response(request)

return middleware

MyMiddleware.async_capable=True
}}}

middleware(request) will return the response in sync case and a coroutine
in the async case, despite being a regular function (because get_response
is a coroutine function in the latter case).

Here is a patch that I use that explains a possible way to fix it:

{{{
def call_mw(mw, request, _call_mw=MiddlewareMixin.__call__):
if isinstance(request, ASGIRequest) and mw.async_capable:
return mw.__acall__(request)
return _call_mw(mw, request)

MiddlewareMixin.__call__ = call_mw
}}}

Github project that shows the error: https://github.com/pwtail/django_bug

--

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