Re: [Django] #33716: async middleware can be a regular function too
Django
-------------------------------+------------------------------------
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, 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
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 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 is_next_middleware_async_capable(mw):
path = f'{mw.__class__.__module__}.{mw.__class__.__name__}'
next_index = settings.MIDDLEWARE.index(path) + 1
mw_class = import_string(settings.MIDDLEWARE[next_index])
return mw_class.async_capable
def call_mw(mw, request, _call_mw=MiddlewareMixin.__call__):
if isinstance(request, ASGIRequest) and
is_next_middleware_async_capable(mw):
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:12>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: abetkin | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 4.0
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Accepted => Unreviewed
--
Ticket URL: <https://code.djangoproject.com/ticket/33716#comment:13>
Django
-------------------------------+--------------------------------------
Reporter: abetkin | Owner: nobody
Component: Uncategorized | Version: 4.0
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Changes (by Carlton Gibson):
* status: new => closed
* resolution: => invalid
Comment:
You're not following the guidance in the
[https://docs.djangoproject.com/en/4.0/topics/http/middleware
/#asynchronous-support Asynchronous support section of the Middle topic
doc].
Specifically:
> The returned callable must match the sync or async nature of the
get_response method. If you have an asynchronous get_response, you must
return a coroutine function (async def).
You're returning a synchronous callable in **both** cases.
See the example of a `@sync_and_async_middleware` given there. Note the
use of `asyncio.iscoroutinefunction(get_response)` to determine what kind
of callable to return.
--
Ticket URL: <https://code.djangoproject.com/ticket/33716#comment:14>
Django
-------------------------------+--------------------------------------
Reporter: abetkin | Owner: nobody
Component: Uncategorized | Version: 4.0
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
* status: closed => new
* type: Bug => New feature
* resolution: invalid =>
Comment:
Okay, this is expected behavior and not a bug. I propose to remove this
restriction for middleware when you have to handle sync and async cases
separately. Because the middleware often does not contain any I/O
--
Ticket URL: <https://code.djangoproject.com/ticket/33716#comment:15>
Django
-------------------------------+--------------------------------------
Reporter: abetkin | Owner: nobody
Component: Uncategorized | Version: 4.0
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
* status: new => closed
* resolution: => wontfix
Comment:
As described it doesn't seem worth the disruption. You can wrap the
(sync?) callable if needed, something like...
{{{
def middleware(request):
...
if asyncio.iscoroutinefunction(get_response):
async def async_middleware(request):
return middleware(request)
return async_middleware
else:
return middleware
}}}
A decorator for that seems feasible.
A bigger disruption would need some further justification. Discussion for
such would need consensus on the DevelopersMailingList.
--
Ticket URL: <https://code.djangoproject.com/ticket/33716#comment:16>