Opened 15 hours ago
Last modified 5 hours ago
#36714 new Bug
Async signals lose ContextVar state due to use of asyncio.gather
| Reported by: | Mykhailo Havelia | Owned by: | |
|---|---|---|---|
| Component: | HTTP handling | Version: | dev |
| Severity: | Normal | Keywords: | asyncio, signals |
| Cc: | Mykhailo Havelia, Carlton Gibson | Triage Stage: | Accepted |
| Has patch: | no | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
The natural way to share global, per-request state in asyncio is through contextvars. In Django, this is typically used via asgiref.local.Local. However, Django's async signal dispatch currently uses asyncio.gather, which internally creates new tasks (asyncio.create_task). This breaks context propagation, since each task gets its own copy of the context. As a result, it's impossible to set a global (context-based) variable inside a signal handler and have it shared with other signal handlers or parts of the same request/response cycle.
Example
from django.core import signals
from django.http import (
HttpRequest,
HttpResponse,
)
import contextvars
from django.urls import path
request_id = contextvars.ContextVar('request_id', default=None)
async def set_global_variable(*args, **kwargs):
# set global variable
request_id.set('request_id_value')
print('get value', request_id.get())
signals.request_started.connect(set_global_variable)
async def index(request: HttpRequest) -> HttpResponse:
# get global variable
print('request_id', request_id.get())
return HttpResponse(content=request_id.get())
urlpatterns = [path("", index), ]
result
get value request_id_value request_id None
The value set inside the signal handler is lost, because the handler runs in a separate task with its own context.
If we are talking exactly about signals.request_started and signals.request_finished, they are typically used for setting up and cleaning up per-request resources. With asyncio.gather, cleanup logic that relies on ContextVar cannot work properly.
from django.core import signals
from django.http import (
HttpRequest,
HttpResponse,
)
import contextvars
from django.urls import path
db_connection = contextvars.ContextVar('db_connection', default=None)
async def get_or_create_connection():
if not db_connection.get():
db_connection.set('connection')
return db_connection.get()
async def close_connection(*args, **kwargs):
connection = db_connection.get()
if not connection:
print('cannot clean - connection does not exist')
return
print('close connection')
connection.set(None)
signals.request_finished.connect(close_connection)
async def index(request: HttpRequest) -> HttpResponse:
# create connection inside handler
connection = await get_or_create_connection()
# await get_data(connection)
return HttpResponse(content="ok")
urlpatterns = [path("", index), ]
result
cannot clean - connection does not exist
Expected behavior
Signal handlers should run in the same async context as the request, preserving ContextVar and asgiref.local.Local state.
Proposed solution
Signal:
Dispatch async signal handlers sequentially (or via direct await) instead of using asyncio.gather, so that the existing execution context is preserved throughout the request lifecycle. Yes, this change removes parallelism, but that shouldn’t be a major concern. The only real benefit of running signal handlers in parallel would be for IO-bound operations - yet in most cases, these handlers interact with the same database connection. Since database operations aren’t truly parallel under the hood, the performance gain from asyncio.gather is negligible.
ASGIHandler:
async def handle(self, scope, receive, send):
...
await signals.request_started.asend(sender=self.__class__, scope=scope)
tasks = [
asyncio.create_task(self.listen_for_disconnect(receive)),
asyncio.create_task(process_request(request, send)),
]
...
await signals.request_finished.asend(sender=self.__class__)
Global variables created inside process_request are not visible to request_finished, because each task runs in a separate context. We can try using contextvars.copy_context() to preserve and share the same context between tasks and signal handlers.
async def handle(self, scope, receive, send):
...
await signals.request_started.asend(sender=self.__class__, scope=scope)
ctx = contextvars.copy_context()
tasks = [
asyncio.create_task(self.listen_for_disconnect(receive)),
asyncio.create_task(process_request(request, send), context=ctx),
]
...
await asyncio.create_task(signals.request_finished.asend(sender=self.__class__), context=ctx)
Here is a simple example
import asyncio
import contextvars
global_state = contextvars.ContextVar('stage', default=0)
async def inc():
value = global_state.get()
print('value: ', value)
global_state.set(value + 1)
async def main():
await asyncio.create_task(inc())
await asyncio.create_task(inc())
await asyncio.create_task(inc())
print('first: ', global_state.get())
ctx = contextvars.copy_context()
await asyncio.create_task(inc(), context=ctx)
await asyncio.create_task(inc(), context=ctx)
await asyncio.create_task(inc(), context=ctx)
print('second: ', ctx.get(global_state))
await main()
result
value: 0 value: 0 value: 0 first: 0 value: 0 value: 1 value: 2 second: 3
Change History (7)
follow-up: 4 comment:1 by , 15 hours ago
| Cc: | added |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
comment:2 by , 13 hours ago
| Owner: | set to |
|---|---|
| Status: | new → assigned |
comment:3 by , 11 hours ago
| Owner: | removed |
|---|---|
| Status: | assigned → new |
comment:4 by , 11 hours ago
Replying to Carlton Gibson:
The whole idea of that API is to allow control over this, when trying to keep things structured, no?
🙂 If we’re talking about ASGIHandler.handler, then yes. It’s easy to implement and would unblock my current work on the async backend.
Then we should resolve #36315 first no?
I think our priority should be to focus first on the:
- database cursor
- ORM
- middlewares,
- signals
- cache
to ensure proper async handling first. After that, we can focus on optimization and improvements since optimization often adds complexity, and there are still some core challenges to solve in the async port 😔.
That said, if I can prepare a small MR with minimal changes that could be reviewed and merged quickly, I’d really prefer that approach, especially since #36315 has been pending for over six months. It would help unblock my current work.
If that’s not possible, I'm more than happy to contribute to #36315 to help move it forward and unblock these changes together.
What do you think?
The whole idea of that API is to allow control over this, when trying to keep things structured, no?
If we're talking about using asyncio.gather inside signals, things get a bit more complicated. I'm not entirely sure how context sharing behaves with parallel tasks yet (I'll need to look into it). We might need to do something similar to what asgiref does, for example:
def _restore_context(context: contextvars.Context) -> None:
cvalue = context.get(cvar)
try:
if cvar.get() != cvalue:
cvar.set(cvalue)
except LookupError:
cvar.set(cvalue)
Manually handling context like this can easily lead to unexpected behavior. For instance, overwriting a global variable when we shouldn't. So we'll need to test this carefully, especially with sync_to_async and async_to_sync. It might take a fair bit of time to review and verify that everything works as expected.
It's much easier to delete it. What do you think?
follow-up: 6 comment:5 by , 11 hours ago
There's already two small PRs for #36315. They look good, and just need a review, and confirmation. (They're sat on my list. 🤹) — My point was that we should resolve those and then implement the change here on top of that, rather than making a separate change here, and then having to redo it.
comment:6 by , 10 hours ago
Replying to Carlton Gibson:
There's already two small PRs for #36315. They look good, and just need a review, and confirmation. (They're sat on my list. 🤹) — My point was that we should resolve those and then implement the change here on top of that, rather than making a separate change here, and then having to redo it.
Got it. I’ll take a look at them tonight and leave a review. Hopefully, that helps speed up the process.
comment:7 by , 5 hours ago
| Type: | Uncategorized → Bug |
|---|
Hi Mykhailo.
Thanks for the report. (And thanks for writing it up so nicely.)
So, yes — passing the
contextexplicitly to the tasks would be my first thought. The whole idea of that API is to allow control over this, when trying to keep things structured, no?Then we should resolve #36315 first no? TaskGroup is the newer preferred API. Do you fancy reviewing that?
I'd be a bit sad if we had to lose the concurrent async dispatch on signals. (I grant your point that it may not be a big gain in performance necessarily, but ... 🙂 )