Opened 4 years ago

Closed 4 years ago

Last modified 4 years 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 by Kordian Kowalski, 4 years ago

Type: UncategorizedBug

comment:2 by Mariusz Felisiak, 4 years ago

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 by Kevin Michel, 4 years ago

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 4 years ago by Kevin Michel (previous) (diff)

comment:4 by Carlton Gibson, 4 years ago

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 by Kevin Michel, 4 years ago

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 by Carlton Gibson, 4 years ago

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 by Mariusz Felisiak, 4 years ago

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

comment:8 by Andrew Godwin, 4 years ago

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 by Mariusz Felisiak, 4 years ago

Triage Stage: AcceptedReady for checkin

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

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 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In ea57a283:

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

comment:12 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

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 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

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