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)

comment:1 by Carlton Gibson, 15 hours ago

Cc: Carlton Gibson added
Triage Stage: UnreviewedAccepted

Hi Mykhailo.

Thanks for the report. (And thanks for writing it up so nicely.)

We can try using contextvars.copy_context() to preserve and share the same context between tasks and signal handlers.

So, yes — passing the context explicitly 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 ... 🙂 )

comment:2 by Varun Kasyap Pentamaraju, 13 hours ago

Owner: set to Varun Kasyap Pentamaraju
Status: newassigned

comment:3 by Varun Kasyap Pentamaraju, 11 hours ago

Owner: Varun Kasyap Pentamaraju removed
Status: assignednew

in reply to:  1 comment:4 by Mykhailo Havelia, 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?

comment:5 by Carlton Gibson, 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.

in reply to:  5 comment:6 by Mykhailo Havelia, 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 Jacob Walls, 5 hours ago

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