Opened 3 weeks ago
Last modified 3 weeks ago
#37177 assigned Bug
Performance issue in Async Middleware handling.
| Reported by: | Carlton Gibson | Owned by: | Jacob Walls |
|---|---|---|---|
| Component: | HTTP handling | Version: | 6.0 |
| Severity: | Normal | Keywords: | async |
| Cc: | Carlton Gibson, Mykhailo Havelia | Triage Stage: | Accepted |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | yes |
| Easy pickings: | no | UI/UX: | no |
Description (last modified by )
Ticket #31224 in commit fc0fa72ff4cdbf5861a366e31cb8bbacd44da22d "Added support for asynchronous views and middleware."
As part of this it added the sync_capable/async_capable flags to MiddlewareMixin and made default middleware advertise async capability via __acall__:
class MiddlewareMixin:
sync_capable = True
async_capable = True
...
async def __acall__(self, request):
"""
Async version of __call__ that is swapped in when an async request
is running.
"""
response = None
if hasattr(self, 'process_request'):
response = await sync_to_async(self.process_request)(request)
response = response or await self.get_response(request)
if hasattr(self, 'process_response'):
response = await sync_to_async(self.process_response)(request, response)
return response
This (unwittingly) undermined the process in load_middleware to minimise the
switches between sync and async contexts.
Every middleware in the default startproject stack (SecurityMiddleware,
SessionMiddleware, CommonMiddleware, CsrfViewMiddleware,
AuthenticationMiddleware, MessageMiddleware, XFrameOptionsMiddleware) is
a MiddlewareMixin subclass that declares async_capable = True but
implements its process_request/process_response/ process_view hooks as
plain synchronous methods. (RemoteUserMiddleware is the lone built-in written
natively async, and it is not in the default stack.)
The middleware chain is built as all async but each middleware then
re-introduces a boundary crossing for each hook — it wraps every sync
process_request/process_response in its own
sync_to_async(thread_sensitive=True).
The net effect inverts the optimiser's intent: a single O(1) bracketing of the
contiguous sync block becomes O(N) per-hook hopping onto the per-request thread
and back.
We end up with essentially 16 context switches reaching the view and back.
If, instead, the default middleware are marked as async_capable = False, we get only 1 such transition during the middleware, as was the intent of the original feature.
Asides:
- We get a second transition for a sync view, as that's wrapped in
sync_to_async. - We only get 0 transitions if the middleware chain is totally native async.
Both of these are expected, and within the performance profile we'd expect.
Django views pretty much always hit the DB, and at that point the additional
thread is already in play. A single sync transition during middle processing is
going to be fine for most use case.
A quick benchmark driving ASGIHangler with Python 3.13, asgiref 3.11, on Django
main (6.2.dev), with middleware marked as async_capable vs not (and an
async-io view doing no more than an await asyncio.sleep(0.005)) shows
significant throughput differences under load:
c=50 seq us/req conc req/s peak thr async mw / /sync/ 1188.9 1331 51 sync mw / /sync/ 291.6 5080 51 async mw / /async/ 1115.1 1508 51 sync mw / /async/ 218.9 6448 51 async mw / /async-io/ 8156.2 1381 51 sync mw / /async-io/ 6684.5 3984 51 c=200 seq us/req conc req/s peak thr async mw / /sync/ 1109.2 898 201 sync mw / /sync/ 400.9 4647 201 async mw / /async/ 1047.8 1436 201 sync mw / /async/ 212.1 6551 201 async mw / /async-io/ 8946.1 1411 201 sync mw / /async-io/ 7111.8 5448 201
Executive summary*: MiddlewareMixin should not declare the default middleware as async_capable.
I'm not sure we can just flip the flag — that's what I did for the benchmark — but maybe MiddlewareMixin could check to see if process_request/process_response were coroutine function or not before just declaring True? (There's probably a little more due diligence to do there too.)
I want to thank Mykhailo Havelia for pointing this issue out. We absolutely shouldn't be transitioning contexts multiple times each way in this case.
Change History (18)
comment:1 by , 3 weeks ago
| Description: | modified (diff) |
|---|
comment:2 by , 3 weeks ago
| Description: | modified (diff) |
|---|
comment:3 by , 3 weeks ago
| Description: | modified (diff) |
|---|
comment:4 by , 3 weeks ago
| Description: | modified (diff) |
|---|
comment:5 by , 3 weeks ago
| Cc: | added |
|---|
comment:6 by , 3 weeks ago
| Owner: | set to |
|---|---|
| Status: | new → assigned |
| Triage Stage: | Unreviewed → Accepted |
follow-ups: 8 13 comment:7 by , 3 weeks ago
@Jacob: great thanks! (I'm off on holiday so won't have time to process it instantly)
My half-thought was, if introspection is too clever:
- Manually mark (affected) built in middleware as
async_capable = False - Deprecate
MiddlewareMixin.__acall__implementation.
And that might be sufficient?
Tests: Locally I patched sync_to_async and async_to_sync calls to count invocations, but that would need to be run alone. 🤔
comment:8 by , 3 weeks ago
Replying to Carlton Gibson:
@Jacob: great thanks! (I'm off on holiday so won't have time to process it instantly)
My half-thought was, if introspection is too clever:
- Manually mark (affected) built in middleware as
async_capable = False- Deprecate
MiddlewareMixin.__acall__implementation.And that might be sufficient?
That's a viable option, but it could break things for people who inherit from the default middleware and rely on async_capable. We'd need to double-check that.
https://github.com/django/new-features/issues/102 is another option for solving this for CPU-bound middleware.
follow-up: 10 comment:9 by , 3 weeks ago
As part of other async benchmarking work I'm doing, I took some time to measure the problem from this ticket, and got consistent results. On a real ASGI server end-to-end (daphne, single process), using the default 7 middlewares, and an async view returning immediately, Python 3.14.5 and asgiref 3.11.1, I tested with ab across concurrency 10 to 200 (-n 8000):
-c │ async_capable=True │ async_capable=False ────┼─────────────────────┼───────────────────── 10 │ 576 req/s │ 920 req/s 50 │ 587 req/s │ 926 req/s 100 │ 570 req/s │ 933 req/s 200 │ 533 req/s │ 883 req/s
Throughput stays flat and latency grows linearly in both (I ran this in my laptop, a saturated single process), so the change shows up as the throughput ceiling: marking the built-in middleware sync-only lifts it ~1.6x at every load level (~1.7 ms to ~1.1 ms per request, WSGI reference, same view: ~1690 req/s.)
follow-up: 11 comment:10 by , 3 weeks ago
Replying to Natalia Bidart:
Throughput stays flat and latency grows linearly
In production I observed somewhat different behaviour. The convoy effect was noticeably stronger under CPU-bound work. We found that past ~40% CPU utilisation, response-time degradation became non-linear, so we had to keep CPU usage below that threshold.
follow-up: 12 comment:11 by , 3 weeks ago
Replying to Mykhailo Havelia:
In production I observed somewhat different behaviour. The convoy effect was noticeably stronger under CPU-bound work. We found that past ~40% CPU utilisation, response-time degradation became non-linear, so we had to keep CPU usage below that threshold.
That's a useful data point, thanks. I think non-linear degradation is consistent with GIL contention getting worse as CPU work rises, which my benchmark setup wouldn't capture: the view I used does no application work, so it really measures the framework/middleware floor. If you can share a benchmark in any reproducible form, I'd really like to measure the CPU-bound case directly rather than infer it.
For reference, here's how I got mine. Single process each, default middleware stack, an async view that returns immediately, loaded with ApacheBench:
# WSGI for baseline sync, other port gunicorn -b 127.0.0.1:9000 -w 1 mysite.wsgi:application # ASGI daphne -b 127.0.0.1 -p 9001 mysite.asgi:application # concurrent requests (change -c 10/50/100/200) ab -n 8000 -c 50 http://127.0.0.1:9001/<async-view>/
follow-up: 16 comment:13 by , 3 weeks ago
Replying to Carlton Gibson:
- Manually mark (affected) built in middleware as
async_capable = False- Deprecate
MiddlewareMixin.__acall__implementation.And that might be sufficient?
I think we should just do 1., as IMO it's a bug in the core middleware, and it's a reasonable user-duty to flip the flag when subclassing to extend with async functionality. Small release note.
I don't see why we have to do 2., MiddlewareMixin is public and __acall__ seems useful.
follow-up: 15 comment:14 by , 3 weeks ago
Replying to Mykhailo Havelia:
I'll send everything I have to Discord
Can you please not use Discord and attach to this ticket instead? Or post in a new forum thread. Discord is not formally used for discussing development of Django, it's mostly used for informal chat and assisting how to use Django.
The right channels for developing Django are this ticket Trac system and the Official Django Forum. Thank you!
comment:15 by , 3 weeks ago
Replying to Natalia Bidart:
The right channels for developing Django are this ticket Trac system and the Official Django Forum. Thank you!
I'm not ready to send it to the forum yet because it's still a bit messy and not finished. So I can't guarantee a timeframe for when it'll be done.
follow-up: 17 comment:16 by , 3 weeks ago
Replying to Jacob Walls:
Replying to Carlton Gibson:
- Manually mark (affected) built in middleware as
async_capable = False- Deprecate
MiddlewareMixin.__acall__implementation.And that might be sufficient?
I think we should just do 1., as IMO it's a bug in the core middleware, and it's a reasonable user-duty to flip the flag when subclassing to extend with async functionality. Small release note.
I don't see why we have to do 2.,
MiddlewareMixinis public and__acall__seems useful.
I was worried about two things.
- Backwards compatibility, for people who extend default Django middlewares and rely on async_capable and/or acall.
- People who don't want to use threads but still want to use at least the default CPU-bound Django middlewares.
To avoid breaking backwards compatibility, I couldn't find a better solution than providing dedicated async middlewares. Anyway, we can split it and do it step by step
comment:17 by , 3 weeks ago
| Has patch: | set |
|---|
Replying to Mykhailo Havelia:
I was worried about two things.
- Backwards compatibility, for people who extend default Django middlewares and rely on async_capable and/or acall.
- People who don't want to use threads but still want to use at least the default CPU-bound Django middlewares.
To avoid breaking backwards compatibility, I couldn't find a better solution than providing dedicated async middlewares. Anyway, we can split it and do it step by step
You're right to consider that, but I think the intended design was that users wouldn't have to swap in async-only middlewares.
On point one, we're talking about asking users who have subclassed the concrete middlewares and implemented their own __acall__() to await get_response(), now having to set async_capable = True. Backward compatibility protections are a bit weaker when you're subclassing.
On point two, by not deprecating MiddlewareMixin.__acall__() entirely, we've left it so the users who don't mind the context switches can get the prior behavior by again, just setting one attribute.
Appreciate any reviews on PR! It does the most minimal thing for now, i.e. "Manually mark (affected) built in middleware as async_capable = False".
Of course, it is not essential to push this into Django 6.1 next week, but I wanted to at least present the option.
comment:18 by , 3 weeks ago
| Patch needs improvement: | set |
|---|
Test suite is deadlocking with this change. Likely we need to solve https://github.com/django/asgiref/issues/535 first.
This was mentioned on the forum at least once, so thanks for the confirmation that this runs counter to the original design.
Just flipping the flag is what a user did, when they saw this in their own middleware, as for them it was a regression, see forum, but given how many releases have followed, introspecting seems like the most backward-compatible thing we can do right now.
Separately, I have a question about how much longer we plan to maintain
MiddlewareMixin.Tentatively assigning to myself, to see if we can advance this before next Wednesday's bug freeze for 6.1.