Skip to content
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

Add signal support for async #96

Open
x0139 opened this issue Jul 2, 2018 · 5 comments
Open

Add signal support for async #96

x0139 opened this issue Jul 2, 2018 · 5 comments

Comments

@x0139
Copy link

x0139 commented Jul 2, 2018

Please add Signal playhouse support on insert method to work with await objects.create()

@rudyryk
Copy link
Member

rudyryk commented Jul 9, 2018

OK, this is up to discuss. Unfortunately, implementation could be tricky because signals are designed to be sync and we need async. And also if someone added sync signals they won't be called from async code. This could be even worse than having no support for signals at all.

@rudyryk
Copy link
Member

rudyryk commented Jul 9, 2018

However, we could try calling sync signals with run_in_executor().

@x0139
Copy link
Author

x0139 commented Jul 10, 2018

There is small problem to call run_in_executor() - await objects.create() uses insert, and in playhouse peewee said:
Peewee signals do not work when you use the Model.insert(), Model.update(), or Model.delete()

@x0139
Copy link
Author

x0139 commented Jul 10, 2018

Why signals with async is a bad idea?

@rudyryk
Copy link
Member

rudyryk commented Jul 10, 2018

I'm not saying this is a bad idea. I just point that it may cause unexpected behaviour and need to be designed properly. Consider someone has added signals in their sync codebase. Should we trigger those signals or not? If we do that, than we have to use run_in_executor(). And yes, we should replicate the logic from sync version as close as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants