Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#32299 closed Bug (fixed)

MiddlewareNotUsed leaves undesired side effects when loading middleware in ASGI context

Reported by: Hubert Bielenia Owned by: Mariusz Felisiak
Component: HTTP handling Version: 3.1
Severity: Release blocker Keywords:
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

I experienced strange issues when working with ASGI , django-debug-toolbar and my own small middleware. It was hard problem to debug, I uploaded an example project here: https://github.com/hbielenia/asgi-djangotoolbar-bug (the name is misleading - I initially thought it's a bug with django-debug-toolbar).

The SESSION_FILE_PATH setting is intentionally broken to cause a 500 error. When starting the application and accessing /admin (any location really, but I wanted to leave it at a minimum and didn't add any views) it gives TypeError: object HttpResponse can't be used in 'await' expression. Commenting out asgi_djangotoolbar_bug.middleware.DummyMiddleware fixes the issue (in that I receive a 500 ImproperlyConfigured exception). I'm not sure about the overall role of django-debug-toolbar here - removing it causes Daphne to return a 500 error page but without debug information and there's no traceback in console either. I decided to leave it since it helped me approximate the causes of issue.

I notice that in https://github.com/django/django/blob/3.1.4/django/core/handlers/base.py#L58 while MiddlewareNotUsed causes the loop to skip futher processing and go to next middleware, it does leave handler variable overwritten with output of self.adapt_method_mode(). On next pass, this handler is passed to next middleware instance, disregarding all the previous checks for (lack of) async support. This likely causes the middleware chain to be "poisoned" from this point onwards, resulting in last middleware in response cycle to return an HttpResponse as a synchronous middleware would, instead of coroutine that is expected.

This is probably avoided by adding async support to my middleware, but unless I'm missing something docs indicate it should work as it is. It is my intention that it's applied only on synchronous requests, so I didn't make it async compatible on purpose. If it's intentional in Django that every middleware needs to support async if the application is run as ASGI app, the documentation should probably state that clearly. Though it kinda defeats the purpose of having async_capable = False flag in the first place.

Change History (5)

comment:1 Changed 2 years ago by Mariusz Felisiak

Cc: Andrew Godwin Carlton Gibson added
Component: UncategorizedHTTP handling
Owner: changed from nobody to Mariusz Felisiak
Severity: NormalRelease blocker
Status: newassigned
Triage Stage: UnreviewedAccepted

Many thanks for the detailed report.

comment:2 Changed 2 years ago by Mariusz Felisiak

Has patch: set

comment:3 Changed 2 years ago by Carlton Gibson

Triage Stage: AcceptedReady for checkin

comment:4 Changed 2 years ago by GitHub <noreply@…>

Resolution: fixed
Status: assignedclosed

In 98ad327:

Fixed #32299 -- Prevented mutating handlers when processing middlewares marking as unused in an async context.

Thanks Hubert Bielenia for the report.

comment:5 Changed 2 years ago by Mariusz Felisiak <felisiak.mariusz@…>

In 6b4b7da7:

[3.1.x] Fixed #32299 -- Prevented mutating handlers when processing middlewares marking as unused in an async context.

Thanks Hubert Bielenia for the report.
Backport of 98ad327864aed8df245fd19ea9d2743279e11643 from master

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