[Django] #34328: Class-based async-only middleware not detected as coroutine in MiddlewareMixin

[Django]

#34328: Class-based async-only middleware not detected as coroutine in
MiddlewareMixin
-------------------------------------+-------------------------------------
Reporter: Sergei | Owner: nobody
Type: Bug | Status: new
Component: | Version: 4.1
Uncategorized | Keywords: middleware async
Severity: Normal | asyncio MiddlewareMixin
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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):

{{{
#!python

>>> class TestMiddleware:
... async def __call__(self, request):
... pass

>>> import asgiref.sync
>>> import inspect
>>> t = TestMiddleware()
>>> asgiref.sync.iscoroutinefunction(t)
False
>>> inspect.iscoroutinefunction(t)
False
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/34328>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#34328: Class-based async-only middleware not detected as coroutine in
MiddlewareMixin
-------------------------------------+-------------------------------------
Reporter: Sergei | Owner: nobody
Type: Bug | Status: new

Component: Uncategorized | Version: 4.1
Severity: Normal | Resolution:
Keywords: middleware async | Triage Stage:
asyncio MiddlewareMixin | Unreviewed
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Sergei):

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 in `MiddlewareMixin`? Or
possibly `asgiref`'s?

By the way, I'm willing to work on this issue.

--
Ticket URL: <https://code.djangoproject.com/ticket/34328#comment:1>

[Django]

#34328: Class-based async-only middleware not detected as coroutine in
MiddlewareMixin
-------------------------------------+-------------------------------------
Reporter: Sergei | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 4.1
Severity: Normal | Resolution:
Keywords: middleware async | Triage Stage:
asyncio MiddlewareMixin | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Sergei):

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
[https://docs.djangoproject.com/en/4.1/topics/http/middleware
/#asynchronous-support the docs] would look:

{{{
#!python

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/34328#comment:2>

[Django]

#34328: Class-based async-only middleware not detected as coroutine in
MiddlewareMixin
-------------------------------------+-------------------------------------
Reporter: Sergei | Owner: nobody

Type: Bug | Status: closed
Component: Uncategorized | Version: 4.1
Severity: Normal | Resolution: invalid

Keywords: middleware async | Triage Stage:
asyncio MiddlewareMixin | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* status: new => closed
* resolution: => invalid

Comment:

As far as I'm aware, you're not following the guidance in the
[https://docs.djangoproject.com/en/dev/topics/http/middleware
/#asynchronous-support "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 to `False`.
> ...
> 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 should set `async_capable` to `True` and handle `get_response` in
`__init__()`, the following works for me:
{{{#!python
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
[https://code.djangoproject.com/wiki/TicketClosingReasons/UseSupportChannels
support channels] if you have further questions.

--
Ticket URL: <https://code.djangoproject.com/ticket/34328#comment:3>

[Django]

#34328: Class-based async-only middleware not detected as coroutine in
MiddlewareMixin
-------------------------------------+-------------------------------------
Reporter: Sergei | Owner: nobody
Type: Bug | Status: closed
Component: Uncategorized | Version: 4.1
Severity: Normal | Resolution: invalid
Keywords: middleware async | Triage Stage:
asyncio MiddlewareMixin | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Sergei):

Replying to [comment:3 Mariusz Felisiak]:

Sorry for not including full example, I just got straight to reasons:

{{{
#!python

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/34328#comment:4>

[Django]

#34328: Class-based async-only middleware not detected as coroutine in
MiddlewareMixin
-------------------------------------+-------------------------------------
Reporter: Sergei | Owner: nobody
Type: Bug | Status: closed
Component: Uncategorized | Version: 4.1
Severity: Normal | Resolution: invalid
Keywords: middleware async | Triage Stage:
asyncio MiddlewareMixin | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* cc: Carlton Gibson (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/34328#comment:5>

[Django]

#34328: Class-based async-only middleware not detected as coroutine in
MiddlewareMixin
-------------------------------------+-------------------------------------
Reporter: Sergei | Owner: nobody

Type: Bug | Status: new
Component: Documentation | Version: 4.1
Severity: Normal | Resolution:
Keywords: middleware async | Triage Stage: Accepted
asyncio MiddlewareMixin |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Carlton Gibson):

* status: closed => new
* resolution: invalid =>
* has_patch: 0 => 1
* component: Uncategorized => Documentation
* stage: Unreviewed => Accepted

Comment:

> It may be preferable to use `inspect.isawaitable` on the result of
`get_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
[https://github.com/python/cpython/blob/e5da9ab2c82c6b4e4f8ffa699a9a609ea1bea255/Lib/test/test_inspect.py#L219-L242
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 [https://github.com/django/django/pull/16554 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.

--
Ticket URL: <https://code.djangoproject.com/ticket/34328#comment:6>

[Django]

#34328: Class-based async-only middleware not detected as coroutine in
MiddlewareMixin
-------------------------------------+-------------------------------------

Reporter: Sergei | Owner: Carlton
| Gibson
Type: Bug | Status: assigned

Component: Documentation | Version: 4.1
Severity: Normal | Resolution:
Keywords: middleware async | Triage Stage: Accepted
asyncio MiddlewareMixin |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Carlton Gibson):

* owner: nobody => Carlton Gibson
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/34328#comment:7>

[Django]

#34328: Class-based async-only middleware not detected as coroutine in
MiddlewareMixin
-------------------------------------+-------------------------------------
Reporter: Sergei | Owner: Carlton

Type: | Gibson
Cleanup/optimization | Status: assigned

Component: Documentation | Version: 4.1
Severity: Normal | Resolution:

Keywords: middleware async | Triage Stage: Ready for
asyncio MiddlewareMixin | checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* type: Bug => Cleanup/optimization
* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/34328#comment:8>

[Django]

#34328: Class-based async-only middleware not detected as coroutine in
MiddlewareMixin
-------------------------------------+-------------------------------------
Reporter: Sergei | Owner: Carlton
Type: | Gibson

Cleanup/optimization | Status: closed
Component: Documentation | Version: 4.1
Severity: Normal | Resolution: fixed

Keywords: middleware async | Triage Stage: Ready for
asyncio MiddlewareMixin | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by GitHub <noreply@…>):

* status: assigned => closed
* resolution: => fixed

Comment:

In [changeset:"ce8189eea007882bbe6db22f86b0965e718bd341" ce8189e]:
{{{
#!CommitTicketReference repository=""
revision="ce8189eea007882bbe6db22f86b0965e718bd341"
Fixed #34328 -- Added async-only class-based middleware example.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/34328#comment:9>

[Django]

#34328: Class-based async-only middleware not detected as coroutine in
MiddlewareMixin
-------------------------------------+-------------------------------------
Reporter: Sergei | Owner: Carlton
Type: | Gibson
Cleanup/optimization | Status: closed
Component: Documentation | Version: 4.1
Severity: Normal | Resolution: fixed
Keywords: middleware async | Triage Stage: Ready for
asyncio MiddlewareMixin | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Carlton Gibson <carlton.gibson@…>):

In [changeset:"b7aab1fb3a41b13ab5d7e19ae9891ed02b9390ce" b7aab1fb]:
{{{
#!CommitTicketReference repository=""
revision="b7aab1fb3a41b13ab5d7e19ae9891ed02b9390ce"
[4.2.x] Fixed #34328 -- Added async-only class-based middleware example.

Backport of ce8189eea007882bbe6db22f86b0965e718bd341 from main
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/34328#comment:10>

[Django]

#34328: Class-based async-only middleware not detected as coroutine in
MiddlewareMixin
-------------------------------------+-------------------------------------
Reporter: Sergei | Owner: Carlton
Type: | Gibson
Cleanup/optimization | Status: closed
Component: Documentation | Version: 4.1
Severity: Normal | Resolution: fixed
Keywords: middleware async | Triage Stage: Ready for
asyncio MiddlewareMixin | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Sergei):

Replying to [comment:6 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 [https://github.com/django/django/pull/16554 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 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`.

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)

{{{#!python
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
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/34328#comment:11>