-
Notifications
You must be signed in to change notification settings - Fork 89
Added a draft introduction for the Hybrid Signal handler #103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| class HybridSignalHandler: | ||
|
|
||
| def sync_call(self, receiver, **kwargs): | ||
| raise NotImplementedError( | ||
| "send is not supported for this handler" | ||
| ) | ||
|
|
||
| async def async_call(self, receiver, **kwargs): | ||
| raise NotImplementedError( | ||
| "asend is not supported for this handler" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, this DEP is a good idea IMO 🚀 I'm a little stuck on this implementation and wanted to see if you had thought of other approaches.
Did you consider revising the @receiver decorator to control whether the handler should be called based on the context? For example:
@receiver(signal, sync=False, _async=True)
async def async_call(receiver, **kwargs):
...
@receiver(signal, sync=True, _async=False)
def sync_call(receiver, **kwargs):
...This would avoid requiring people to use a inheritance in their handlers and still provide the flexibility you're looking for. I haven't thought this entirely through though, so I could be missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered that approach, but it's much easier to make mistakes with it. In most cases, a handler should run regardless of whether the context is async or sync. For example:
@receiver(request_finished, with_async=False)
def close_db_connection(sender, **kwargs):
# close sync DB connection
...When ASGIHandler calls signals.request_finished.asend(sender=self.__class__), Django will not close a database connection that was created inside sync_to_async.
async def index(request):
# do async work
...
await sync_to_async(do_sync_work_with_sync_db)()Additionally, even if production uses only async connections, the development server wraps all async views with async_to_sync and still sends request_finished synchronously.
That’s why I think using a class-based design is a better choice. It encourages developers to implement both async and sync versions explicitly, and Django can easily raise an exception if one is missing.
We can skip inheritance and just use duck typing (if an object has sync_call and async_call, then it is a hybrid handler) 😁
I don’t know, I just kind of like the idea of using classes — it feels like a better fit 😅
class LogginHandler:
def log(self):
print('done')
def sync_call(self, receiver, **kwargs):
self.log()
async def async_call(self, receiver, **kwargs):
self.log()There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it seems like we want the accept a pair of callables, for the sync and async case respectively, rather than just the one.
The old fashioned way to do that might have been to pass a tuple...
receiver(request_finished)((sync_version, async_version))
(Or similar)
Tuples are OK until you need to remember which field stands for what. Is it sync, async or async, sync?
So we have NamedTuples, or these days dataclasses:
from dataclasses import dataclass
from typing import Callable
@dataclass
class MyReceiver:
sync_handler: Callable
async_handler: Callable
receiver(request_finished)(MyReceiver(sync_handler=sync_version, async_handler=async_version))
(Or similar)
Here the data class is just a namespace for the pair of callables we want to pass.
If we used a runtime checkable protocol in receiver, folks would be free to define a regular class using bound methods, rather than free functions, if the additional class behaviour (state on self essentially) was of use.
(Or something along those lines)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could leave the receiver decorator untouched and make changes only to the signal API(decorator is only useful for wrapping a function or a class).
request_finished.hybrid_connect(async_handler=async_version, sync_handler=sync_version)However, I prefer using a class-based approach. In my experience, it's much easier to keep things organized this way, sync and async versions often share common logic, and with a class, it's simple to move that into a method with proper encapsulation.
FieldExtension from the strawberry is a perfect example of how it can look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there's no reason to not allow people to use a class, especially if there's shared logic as you say.
My proposal was to define a protocol that would also allow named pairs of receivers (which is conceptually what's needed).
Then you pass either a single callable (as now) or an instance of the protocol (to provide both versions). We already check what kind of receiver was passed at connection, so it's just another branch to handle the both case suggested here.
Does that make sense? Maybe I'm missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I just wanted to emphasize that receiver is a decorator that simply calls the connect method under the hood. So, I think we can keep it as it is and just add another method for connecting to the signal.
request_finished.hybrid_connect(async_handler=async_version, sync_handler=sync_version)or
request_finished.hybrid_connect(handler_obj)- we don't need provide dataclass
- we don't need to change default
receiver's behaviour
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. And kwargs might serve the same purpose of keeping which callable is which clear in the code.
(These seem like colour of the bike shed questions. We don't have to settle them exactly at this point 🙂)
|
I think the basic idea here is correct @Arfey here: No-reason at all why built-in signals, and those from third-party apps, shouldn't have a way to opt-in to providing both sync and async versions of the same signal handler. 👍 |
Introduce a draft proposal for the Hybrid Signal handler, which allows defining separate sync and async implementations for signal receivers to improve performance and simplify mixed context execution.