#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 , 3 years ago
comment:2 by , 3 years 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 , 3 years 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_capableis a boolean indicating if the middleware can handle asynchronous requests. Defaults toFalse....
The returned callable must match the sync or async nature of theget_responsemethod. 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 , 3 years 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 , 3 years ago
| Cc: | added |
|---|
follow-up: 11 comment:6 by , 3 years ago
| Component: | Uncategorized → Documentation |
|---|---|
| Has patch: | set |
| Resolution: | invalid |
| Status: | closed → new |
| Triage Stage: | Unreviewed → Accepted |
It may be preferable to use
inspect.isawaitableon 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 , 3 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
comment:8 by , 3 years ago
| Triage Stage: | Accepted → Ready for checkin |
|---|---|
| Type: | Bug → Cleanup/optimization |
comment:11 by , 3 years ago
Replying to Carlton Gibson:
Perhaps adding a class example like yours, and emphasising that your middleware must correctly respond to
iscoroutinefunctionwould 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_responseand usingiscoroutinewould 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.