Opened 4 years ago

Closed 20 months ago

Last modified 20 months ago

#32172 closed New feature (fixed)

Adapt signals to allow async handlers

Reported by: Carlton Gibson Owned by: Jon Janzen
Component: Core (Other) Version: dev
Severity: Normal Keywords: async
Cc: Andrew Godwin, Jon Janzen 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 (last modified by Carlton Gibson)

Currently (circa 3.1) we have a couple of sync_to_async() usages in the main line of `ASGIHandler.__call__()`, for the request life cycle signals:

        await sync_to_async(signals.request_started.send, thread_sensitive=True)(sender=self.__class__, scope=scope)

Much as is done of the middleware, adapting to accept either synchronous or asynchronous middleware classes, it would be good to be able to push that sync_to_async() call down into the send() method, allowing signal handlers to be either sync (as now) or async coroutines.

This would give us the option of having a completely async request pathway, if we were to remove all synchronous middleware and signal handlers.

There will need to be some discussion of exact implementation, perhaps on a PR, or the Forum's Async category but opening this ticket to track efforts.

Change History (15)

comment:1 by Konstantin Volkov, 4 years ago

Owner: changed from nobody to Konstantin Volkov
Status: newassigned

comment:2 by Mariusz Felisiak, 4 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Carlton Gibson, 4 years ago

Description: modified (diff)

comment:4 by Carlton Gibson, 4 years ago

Has patch: set

comment:5 by Carlton Gibson, 4 years ago

Triage Stage: AcceptedReady for checkin

I'm going to mark this RFC — I don't think it's far away.

We want to do a small amount of profiling before we say the final OK, but it can live off the review list for a while during that.

comment:6 by Carlton Gibson, 4 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Performance issues are not looking good at first glance — so we need a proper story there to move forward. Will mark as PNI whilst we discuss.

comment:7 by Carlton Gibson, 2 years ago

Has patch: unset
Patch needs improvement: unset

comment:8 by Jon Janzen, 21 months ago

Cc: Jon Janzen added

I hope I'm not spamming this ticket, but I posted on the Forum about asyncifying the auth app which would involve this ticket so I thought it would be a good idea to CC the ticket on this issue: https://forum.djangoproject.com/t/asyncifying-django-contrib-auth-and-signals-and-maybe-sessions/18770

comment:9 by Jon Janzen, 21 months ago

Hey Konstantin,

I'm interested in working on this ticket, would you mind if I claimed it?

comment:10 by Jon Janzen, 21 months ago

Owner: changed from Konstantin Volkov to Jon Janzen

comment:11 by Carlton Gibson, 21 months ago

The new PR looks promising: any async receivers are dispatch concurrently.

I'll (briefly) see if others have capacity to review, otherwise I'd be inclined to push it forwards.

comment:12 by Carlton Gibson, 21 months ago

Has patch: set

comment:13 by Mariusz Felisiak, 20 months ago

Triage Stage: AcceptedReady for checkin

comment:14 by Mariusz Felisiak <felisiak.mariusz@…>, 20 months ago

Resolution: fixed
Status: assignedclosed

In e83a8856:

Fixed #32172 -- Adapted signals to allow async handlers.

co-authored-by: kozzztik <kozzztik@…>
co-authored-by: Carlton Gibson <carlton.gibson@…>

comment:15 by GitHub <noreply@…>, 20 months ago

In 9b224579:

Refs #32172 -- Used asgiref coroutine shim in async signals tests.

Bug in e83a88566a71a2353cebc35992c110be0f8628af.

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