Opened 2 years ago

Closed 2 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 abetkin)

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)

bug.png (92.3 KB ) - added by abetkin 2 years ago.

Download all attachments as: .zip

Change History (17)

by abetkin, 2 years ago

Attachment: bug.png added

comment:1 by abetkin, 2 years ago

Description: modified (diff)

comment:2 by abetkin, 2 years ago

Description: modified (diff)

comment:3 by abetkin, 2 years ago

Description: modified (diff)
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

comment:4 by abetkin, 2 years ago

Resolution: invalid
Status: newclosed

comment:5 by abetkin, 2 years ago

Severity: Release blockerNormal

comment:6 by abetkin, 2 years ago

Description: modified (diff)
Resolution: invalid
Status: closednew
Summary: Async-capable middleware is not async-capableasync get_response can be a regular function too

comment:7 by abetkin, 2 years ago

Severity: NormalRelease blocker

comment:8 by abetkin, 2 years ago

Severity: Release blockerNormal

comment:9 by abetkin, 2 years ago

Description: modified (diff)

comment:10 by abetkin, 2 years ago

Description: modified (diff)

comment:11 by abetkin, 2 years ago

Summary: async get_response can be a regular function tooasync middleware can be a regular function too

comment:12 by abetkin, 2 years ago

Description: modified (diff)

comment:13 by Carlton Gibson, 2 years ago

Triage Stage: AcceptedUnreviewed

comment:14 by Carlton Gibson, 2 years ago

Resolution: invalid
Status: newclosed

You're not following the guidance in the Asynchronous support section of the Middleware topic doc.

Specifically:

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're returning a synchronous callable in both cases.

See the example of a @sync_and_async_middleware given there. Note the use of asyncio.iscoroutinefunction(get_response) to determine what kind of callable to return.

Last edited 2 years ago by Carlton Gibson (previous) (diff)

comment:15 by abetkin, 2 years ago

Resolution: invalid
Status: closednew
Type: BugNew 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

Last edited 2 years ago by abetkin (previous) (diff)

comment:16 by Carlton Gibson, 2 years ago

Resolution: wontfix
Status: newclosed

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.

Note: See TracTickets for help on using tickets.
Back to Top