Opened 3 years ago
Closed 3 years ago
#33716 closed New feature (wontfix)
async middleware can be a regular function too
Reported by: | abetkin | Owned by: | nobody |
---|---|---|---|
Component: | Uncategorized | Version: | 4.0 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Unreviewed | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
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
Attachments (1)
Change History (17)
by , 3 years ago
comment:1 by , 3 years ago
Description: | modified (diff) |
---|
comment:2 by , 3 years ago
Description: | modified (diff) |
---|
comment:3 by , 3 years ago
Description: | modified (diff) |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Bug |
comment:4 by , 3 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
comment:5 by , 3 years ago
Severity: | Release blocker → Normal |
---|
comment:6 by , 3 years ago
Description: | modified (diff) |
---|---|
Resolution: | invalid |
Status: | closed → new |
Summary: | Async-capable middleware is not async-capable → async get_response can be a regular function too |
comment:7 by , 3 years ago
Severity: | Normal → Release blocker |
---|
comment:8 by , 3 years ago
Severity: | Release blocker → Normal |
---|
comment:9 by , 3 years ago
Description: | modified (diff) |
---|
comment:10 by , 3 years ago
Description: | modified (diff) |
---|
comment:11 by , 3 years ago
Summary: | async get_response can be a regular function too → async middleware can be a regular function too |
---|
comment:12 by , 3 years ago
Description: | modified (diff) |
---|
comment:13 by , 3 years ago
Triage Stage: | Accepted → Unreviewed |
---|
comment:14 by , 3 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
comment:15 by , 3 years ago
Resolution: | invalid |
---|---|
Status: | closed → new |
Type: | Bug → New feature |
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.
Because the check is really redundant. We do know whether the next middleware is async-capable from its definition.
This is a weird API. The user has to return sync or async function based on the type of the next middleware function. Well, python is duck-typed, you cannot rely on types
comment:16 by , 3 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
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.
You're not following the guidance in the Asynchronous support section of the Middleware topic doc.
Specifically:
You're returning a synchronous callable in both cases.
See the example of a
@sync_and_async_middleware
given there. Note the use ofasyncio.iscoroutinefunction(get_response)
to determine what kind of callable to return.