[Django] #31928: ASGI, coroutine passed to the first middleware's process_response instead of response object

[Django]

#31928: ASGI, coroutine passed to the first middleware's process_response instead
of response object
--------------------------------------------+------------------------
Reporter: Kordian Kowalski | Owner: nobody
Type: Uncategorized | Status: new
Component: HTTP handling | Version: 3.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
--------------------------------------------+------------------------
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'> }}}

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

[Django]

#31928: ASGI, coroutine passed to the first middleware's process_response instead
of response object

----------------------------------+--------------------------------------

Reporter: Kordian Kowalski | Owner: nobody

Type: Bug | Status: new

Component: HTTP handling | Version: 3.1

Severity: Normal | Resolution:

Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

----------------------------------+--------------------------------------
Changes (by Kordian Kowalski):

* type: Uncategorized => Bug

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

[Django]

#31928: Coroutine passed to the first middleware's process_response() instead of
HttpResponse.
----------------------------------+------------------------------------

Reporter: Kordian Kowalski | Owner: nobody

Type: Bug | Status: new

Component: HTTP handling | Version: 3.1

Severity: Release blocker | Resolution:
Keywords: async asgi | Triage Stage: Accepted

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

----------------------------------+------------------------------------
Changes (by felixxm):

* cc: Andrew Godwin, Carlton Gibson (added)
* keywords: => async asgi
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted

Comment:

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?

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

[Django]

#31928: Coroutine passed to the first middleware's process_response() instead of
HttpResponse.
----------------------------------+------------------------------------

Reporter: Kordian Kowalski | Owner: nobody

Type: Bug | Status: new

Component: HTTP handling | Version: 3.1

Severity: Release blocker | Resolution:
Keywords: async asgi | Triage Stage: Accepted

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

----------------------------------+------------------------------------

Comment (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.

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

[Django]

#31928: Coroutine passed to the first middleware's process_response() instead of
HttpResponse.
----------------------------------+------------------------------------

Reporter: Kordian Kowalski | Owner: nobody

Type: Bug | Status: new

Component: HTTP handling | Version: 3.1

Severity: Release blocker | Resolution:
Keywords: async asgi | Triage Stage: Accepted

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

----------------------------------+------------------------------------

Comment (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

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

[Django]

#31928: Coroutine passed to the first middleware's process_response() instead of
HttpResponse.
----------------------------------+------------------------------------

Reporter: Kordian Kowalski | Owner: nobody

Type: Bug | Status: new

Component: HTTP handling | Version: 3.1

Severity: Release blocker | Resolution:
Keywords: async asgi | Triage Stage: Accepted

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

----------------------------------+------------------------------------

Comment (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.

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

[Django]

#31928: Coroutine passed to the first middleware's process_response() instead of
HttpResponse.
----------------------------------+------------------------------------

Reporter: Kordian Kowalski | Owner: nobody

Type: Bug | Status: new

Component: HTTP handling | Version: 3.1

Severity: Release blocker | Resolution:
Keywords: async asgi | Triage Stage: Accepted

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

----------------------------------+------------------------------------

Comment (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. 👌

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

[Django]

#31928: Coroutine passed to the first middleware's process_response() instead of
HttpResponse.
----------------------------------+----------------------------------------
Reporter: Kordian Kowalski | Owner: Kevin Michel
Type: Bug | Status: assigned

Component: HTTP handling | Version: 3.1

Severity: Release blocker | Resolution:
Keywords: async asgi | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

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

----------------------------------+----------------------------------------

Comment (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.

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

[Django]

#31928: Coroutine passed to the first middleware's process_response() instead of
HttpResponse.
-------------------------------------+-------------------------------------

Reporter: Kordian Kowalski | Owner: Kevin
| Michel
Type: Bug | Status: assigned

Component: HTTP handling | Version: 3.1

Severity: Release blocker | Resolution:
Keywords: async asgi | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0

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

-------------------------------------+-------------------------------------
Changes (by felixxm):

* stage: Accepted => Ready for checkin

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

[Django]

#31928: Coroutine passed to the first middleware's process_response() instead of
HttpResponse.
-------------------------------------+-------------------------------------
Reporter: Kordian Kowalski | Owner: Kevin
| Michel

Type: Bug | Status: closed

Component: HTTP handling | Version: 3.1

Severity: Release blocker | Resolution: fixed

Keywords: async asgi | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

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

-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak <felisiak.mariusz@…>):

In [changeset:"825ce75faec63ce81601e31152c757a9c28fed13" 825ce75f]:
{{{
#!CommitTicketReference repository=""
revision="825ce75faec63ce81601e31152c757a9c28fed13"
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.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/31928#comment:12>

[Django]

#31928: Coroutine passed to the first middleware's process_response() instead of
HttpResponse.
-------------------------------------+-------------------------------------
Reporter: Kordian Kowalski | Owner: Kevin
| Michel
Type: Bug | Status: closed

Component: HTTP handling | Version: 3.1

Severity: Release blocker | Resolution: fixed
Keywords: async asgi | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

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

-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak <felisiak.mariusz@…>):

In [changeset:"ea57a2834fe32d895e6e6b0f3791feb2fec71737" ea57a283]:
{{{
#!CommitTicketReference repository=""
revision="ea57a2834fe32d895e6e6b0f3791feb2fec71737"
Refs #31928 -- Made SessionMiddleware call super().__init__().
}}}

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

[Django]

#31928: Coroutine passed to the first middleware's process_response() instead of
HttpResponse.
-------------------------------------+-------------------------------------
Reporter: Kordian Kowalski | Owner: Kevin
| Michel
Type: Bug | Status: closed

Component: HTTP handling | Version: 3.1

Severity: Release blocker | Resolution: fixed
Keywords: async asgi | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

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

-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):

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

Comment:

In [changeset:"3a42c0447b6a3187bd2aaae0bba566d835c47d22" 3a42c044]:
{{{
#!CommitTicketReference repository=""
revision="3a42c0447b6a3187bd2aaae0bba566d835c47d22"
[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
}}}

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

[Django]

#31928: Coroutine passed to the first middleware's process_response() instead of
HttpResponse.
-------------------------------------+-------------------------------------
Reporter: Kordian Kowalski | Owner: Kevin
| Michel
Type: Bug | Status: closed

Component: HTTP handling | Version: 3.1

Severity: Release blocker | Resolution: fixed
Keywords: async asgi | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

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

-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak <felisiak.mariusz@…>):

In [changeset:"225261b70136fa90e63b6cf4ea10341e793d7341" 225261b7]:
{{{
#!CommitTicketReference repository=""
revision="225261b70136fa90e63b6cf4ea10341e793d7341"
Refs #31928 -- Added various middlewares tests for detecting when
get_response is coroutine.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/31928#comment:13>