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_middlewaregiven there. Note the use ofasyncio.iscoroutinefunction(get_response)to determine what kind of callable to return.