#37177: Performance issue in Async Middleware handling.
--------------------------------+--------------------------------------
Reporter: Carlton Gibson | Owner: (none)
Type: Bug | Status: new
Component: HTTP handling | Version: 6.0
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+--------------------------------------
Description changed by Carlton Gibson:
Old description:
> 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 before reaching the view.
>
> 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 default 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. The
> conclusion to remove the async support entirely goes too far I think, but
> we absolutely shouldn't be transitioning contexts multiple times each way
> in this case.
New description:
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 before reaching the view.
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. The
conclusion to remove the async support entirely goes too far I think, but
we absolutely shouldn't be transitioning contexts multiple times each way
in this case.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/37177#comment:1>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion visit
https://groups.google.com/d/msgid/django-updates/0107019edabd4c26-3d993013-486e-4341-a493-4319812010a1-000000%40eu-central-1.amazonses.com.
