#34328 closed Cleanup/optimization (fixed)
Class-based async-only middleware not detected as coroutine in MiddlewareMixin
Reported by: | Sergei | Owned by: | Carlton Gibson |
---|---|---|---|
Component: | Documentation | Version: | 4.1 |
Severity: | Normal | Keywords: | middleware async asyncio MiddlewareMixin |
Cc: | Carlton Gibson | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
If an async middleware is implemented in class-based manner, other middleware in a chain may fail to recognize it as async. Stock Django middlewares use MiddlewareMixin
, and it does not check for this kind of implementation.
This results in coroutines leaking to sync code, and AttributeError
's raised for accessing things like .status_code
on a coroutine object instead of a response.
MiddlewareMixin._async_check()
uses asgiref.sync.iscoroutinefunction()
for checking.
Small example (Python 3.10):
>>> class TestMiddleware: ... async def __call__(self, request): ... pass >>> import asgiref.sync >>> import inspect >>> t = TestMiddleware() >>> asgiref.sync.iscoroutinefunction(t) False >>> inspect.iscoroutinefunction(t) False
Change History (11)
comment:1 by , 21 months ago
comment:2 by , 21 months ago
I've been thinking a bit more about it and going through Django code. MiddlewareMixin
checks passed get_response
callable for being async. It may be preferable to use inspect.isawaitable
on the result of get_response
(more robust).
But with this approach I'm unsure how the example from the docs would look:
import asyncio from django.utils.decorators import sync_and_async_middleware @sync_and_async_middleware def simple_middleware(get_response): # One-time configuration and initialization goes here. if asyncio.iscoroutinefunction(get_response): async def middleware(request): # Do something here! response = await get_response(request) return response else: def middleware(request): # Do something here! response = get_response(request) return response return middleware
Right now it returns function which is semantically sync or async. With inspect.isawaitable
approach it would be only one function all the time.
Possible alternative: check for get_response.__call__
attribute and run asgiref.sync.iscoroutinefunction
on it.
follow-up: 4 comment:3 by , 21 months ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
As far as I'm aware, you're not following the guidance in the Asynchronous support docs:
To change these assumptions, set the following attributes on your middleware factory function or class:
async_capable
is a boolean indicating if the middleware can handle asynchronous requests. Defaults toFalse
....
The returned callable must match the sync or async nature of theget_response
method. If you have an asynchronousget_response
, you must return a coroutine function (async def
).
You should set async_capable
to True
and handle get_response
in __init__()
, the following works for me:
class AsyncMiddleware: async_capable = True def __init__(self, get_response): self.get_response = get_response if iscoroutinefunction(self.get_response): markcoroutinefunction(self) async def __call__(self, request): response = await self.get_response(request) # Some logic ... return response
Please use one of support channels if you have further questions.
comment:4 by , 21 months ago
Replying to Mariusz Felisiak:
Sorry for not including full example, I just got straight to reasons:
class OVFXIDAuthMiddleware: sync_capable = False async_capable = True def __init__(self, get_response): self.get_response = get_response async def __call__(self, request): return await self.get_response(request)
The docs state:
If your middleware has both sync_capable = True and async_capable = True, then Django will pass it the request without converting it. In this case, you can work out if your middleware will receive async requests by checking if the get_response object you are passed is a coroutine function, using asgiref.sync.iscoroutinefunction.
According to the doc, if I state only async, I do not need to use iscoroutinefunction
and mark my instance with markcoroutinefunction
(Django should handle it), but that results in exceptions. Therefore I reported it as a bug, sorry if I misunderstood something.
To clarify a bit more: yes, my middleware got get_response
as a coroutine correctly, however problems arise later in the chain - later middleware does not detect my middleware as a coroutine-function. markcoroutinefunction(self)
in my __init__
obviously helps, as well as implementing my middleware in functional style.
comment:5 by , 21 months ago
Cc: | added |
---|
follow-up: 11 comment:6 by , 21 months ago
Component: | Uncategorized → Documentation |
---|---|
Has patch: | set |
Resolution: | invalid |
Status: | closed → new |
Triage Stage: | Unreviewed → Accepted |
It may be preferable to use
inspect.isawaitable
on the result ofget_response
(more robust).
Calling the wrapped get_response
and using iscoroutine
would trigger its side-effects — it would run the middleware in the simplest case.
That's not available, when building the middleware chain, outside of the request-response cycle, which is why we need to use iscoroutinefunction
.
It's a limitation of iscoroutinefunction
that it does not detect async def __call__ implementations, unless the instance is explicitly marked — so use of markcoroutinefunction(self)
as in MiddlewareMixin
is required.
Perhaps adding a class example like yours, and emphasising that your middleware must correctly respond to iscoroutinefunction
would be helpful.
I've created a PR for that here. (We can re-close if that's not right.)
The sync_capable
and async_capable
markers are informative for Django — they tell it whether it needs to adapt the incoming get_response
for the middleware or not. If you have a an async only middleware, setting sync_capable = False
async_capable = True
as you did is correct.
In this case, you can work out if your middleware will receive async requests by checking if the get_response object you are passed is a coroutine function, using asgiref.sync.iscoroutinefunction.
This refers to branching within your middleware. The issue you're hitting concerns how your middleware looks from the outside.
comment:7 by , 21 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:8 by , 21 months ago
Triage Stage: | Accepted → Ready for checkin |
---|---|
Type: | Bug → Cleanup/optimization |
comment:11 by , 21 months ago
Replying to Carlton Gibson:
Perhaps adding a class example like yours, and emphasising that your middleware must correctly respond to
iscoroutinefunction
would be helpful.
I've created a PR for that here. (We can re-close if that's not right.)
Thanks, the doc addition looks good!
Calling the wrapped
get_response
and usingiscoroutine
would trigger its side-effects — it would run the middleware in the simplest case.
That's not available, when building the middleware chain, outside of the request-response cycle, which is why we need to useiscoroutinefunction
.
I had thought about branching during middleware running, not at chain build time. But as I've tried to lay out such a middleware, the downsides became apparent. Totally not worth it.
Not sure if it contributes anything, but below is an example (a tiny research probably?).
The downsides are apparent:
- not possible to do any pre-processing of request in async mode
- need to rework bundled middleware, as well as any sync-async middleware in the wild (yikes)
class SyncAndAsyncMiddleware: async_capable = True sync_capable = True def __init__(self, get_response): self.get_response = get_response def __call__(self, request): # pre-logic (unknown mode) # only non-blocking operations in sync mode are possible here response = self.get_response(request) if inspect.isawaitable(response): return self._async_call(response) # post logic (sync mode) return response async def _async_call(self, response): # too late for any pre-logic on request response = await response # some post-logic (async mode) return response
I'm not even sure if it's a documentation issue - i.e. should it mention adding
asgiref.sync.markcoroutinefunction(self)
call to your own class-based middleware? Or is it async detection issue inMiddlewareMixin
? Or possiblyasgiref
's?By the way, I'm willing to work on this issue.