Opened 9 months ago

Closed 8 months ago

Last modified 8 months ago

#31928 closed Bug (fixed)

Coroutine passed to the first middleware's process_response() instead of HttpResponse.

Reported by: Kordian Kowalski Owned by: Kevin Michel
Component: HTTP handling Version: 3.1
Severity: Release blocker Keywords: async asgi
Cc: Andrew Godwin, 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

Like the title says, using ASGI (+ uvicorn in my case), the first middleware (according to the list in settings.py) receives a coroutine as its response parameter, while all other middlewares down the line receive a django.http.response.HttpResponse object.

This seems to have caused an issue in the django-cors-headers package which is often placed first in order:
https://github.com/adamchainz/django-cors-headers/issues/558

How to reproduce:

  • Set up a django 3.1 project with an async server (uvicorn in my case)
  • Create a dummy class-based middleware that prints the types of arguments it receives in its process_response method:
class DummyMiddleware(MiddlewareMixin):
    def process_response(self, request, response):
        print(request.__class__, response.__class__)
  • Set up the middleware as the first one in settings.py:
MIDDLEWARE = [
    'django_uvicorn_test.middleware.DummyMiddleware',
    'django.middleware.security.SecurityMiddleware',
   ...
  • Launch the server and perform any request, observe console output:

<class 'django.core.handlers.asgi.ASGIRequest'> <class 'coroutine'>

  • Move the middleware down on the list, restart the server and perform a request again:

<class 'django.core.handlers.asgi.ASGIRequest'> <class 'django.http.response.HttpResponse'>

Change History (13)

comment:1 Changed 9 months ago by Kordian Kowalski

Type: UncategorizedBug

comment:2 Changed 9 months ago by Mariusz Felisiak

Cc: Andrew Godwin Carlton Gibson added
Keywords: async asgi added
Severity: NormalRelease blocker
Summary: ASGI, coroutine passed to the first middleware's process_response instead of response objectCoroutine passed to the first middleware's process_response() instead of HttpResponse.
Triage Stage: UnreviewedAccepted

Tentatively accepted for investigation. It's not about the first middleware because if you have only one it gets HttpResponse, but if you have at least two then then the first in a chain gets coroutine.

Andrew, Can you take a look?

comment:3 Changed 9 months ago by Kevin Michel

I think it's a bug in SecurityMiddleware : its __init__ does not call super().__init__().

This causes MiddlewareMixin._async_check() to not be called during init and the
_is_coroutine magic attribute to be missing despite the async_capable=True
declaration.

Last edited 9 months ago by Kevin Michel (previous) (diff)

comment:4 Changed 9 months ago by Carlton Gibson

Hi Kevin. That's a good spot! :)

Would you fancy taking on a quick PR for this? (It's on my list if not but...)
Thanks

comment:5 Changed 9 months ago by Kevin Michel

Hi !

Yeah I can do a PR for that :)

I see that the cache-related middlewares have the same issue,
would you prefer a common PR with all middlewares fixed or should I make a separate PR for the cache ?

The 3 cache middlewares (UpdateCacheMiddleware, FetchFromCacheMiddleware, CacheMiddleware)
will probably need to be changed together since they are related by inheritance.

comment:6 Changed 9 months ago by Carlton Gibson

Hi Kevin.

would you prefer a common PR with all middlewares fixed or should I make a separate PR for the cache ?

It’s a single issue no, a single PR, should be fine. (We can maybe do three commits if that seems better, but it’s a single issue no? :)

I didn’t dig in yet, so you’re ahead of me but, if you can narrow down the issue in a test case (or three...) then that’s the main thing I’d think.

Thanks for the input! Super. 👌

comment:7 Changed 9 months ago by Mariusz Felisiak

Has patch: set
Owner: changed from nobody to Kevin Michel
Status: newassigned

comment:8 Changed 9 months ago by Andrew Godwin

Nice catch - can't believe I didn't see this during testing. I agree with the diagnosis of the problem, too, I'll go take a look at the PR.

comment:9 Changed 8 months ago by Mariusz Felisiak

Triage Stage: AcceptedReady for checkin

comment:10 Changed 8 months ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In 3a42c044:

[3.1.x] Fixed #31928 -- Fixed detecting an async get_response in various middlewares.

SecurityMiddleware and the three cache middlewares were not calling
super().init() during their initialization or calling the required
MiddlewareMixin._async_check() method.

This made the middlewares not properly present as coroutine and
confused the middleware chain when used in a fully async context.

Thanks Kordian Kowalski for the report.

Backport of 825ce75faec63ce81601e31152c757a9c28fed13 from master

comment:11 Changed 8 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In ea57a283:

Refs #31928 -- Made SessionMiddleware call super().init().

comment:12 Changed 8 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 825ce75f:

Fixed #31928 -- Fixed detecting an async get_response in various middlewares.

SecurityMiddleware and the three cache middlewares were not calling
super().init() during their initialization or calling the required
MiddlewareMixin._async_check() method.

This made the middlewares not properly present as coroutine and
confused the middleware chain when used in a fully async context.

Thanks Kordian Kowalski for the report.

comment:13 Changed 8 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 225261b7:

Refs #31928 -- Added various middlewares tests for detecting when get_response is coroutine.

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