Opened 22 months ago

Closed 22 months ago

Last modified 22 months ago

#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 Sergei, 22 months ago

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.

comment:2 by Sergei, 22 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.

comment:3 by Mariusz Felisiak, 22 months ago

Resolution: invalid
Status: newclosed

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 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:

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.

in reply to:  3 comment:4 by Sergei, 22 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 Mariusz Felisiak, 22 months ago

Cc: Carlton Gibson added

comment:6 by Carlton Gibson, 22 months ago

Component: UncategorizedDocumentation
Has patch: set
Resolution: invalid
Status: closednew
Triage Stage: UnreviewedAccepted

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 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 Carlton Gibson, 22 months ago

Owner: changed from nobody to Carlton Gibson
Status: newassigned

comment:8 by Mariusz Felisiak, 22 months ago

Triage Stage: AcceptedReady for checkin
Type: BugCleanup/optimization

comment:9 by GitHub <noreply@…>, 22 months ago

Resolution: fixed
Status: assignedclosed

In ce8189e:

Fixed #34328 -- Added async-only class-based middleware example.

comment:10 by Carlton Gibson <carlton.gibson@…>, 22 months ago

In b7aab1fb:

[4.2.x] Fixed #34328 -- Added async-only class-based middleware example.

Backport of ce8189eea007882bbe6db22f86b0965e718bd341 from main

in reply to:  6 comment:11 by Sergei, 22 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 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)
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
Note: See TracTickets for help on using tickets.
Back to Top