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 async support in WS #134

Merged
merged 7 commits into from
Dec 20, 2021
Merged

Add async support in WS #134

merged 7 commits into from
Dec 20, 2021

Conversation

sansyrox
Copy link
Member

@sansyrox sansyrox commented Dec 15, 2021

Description

This PR fixes #116

@netlify
Copy link

netlify bot commented Dec 15, 2021

✔️ Deploy Preview for robyn canceled.

🔨 Explore the source changes: 6a3c22e

🔍 Inspect the deploy log: https://app.netlify.com/sites/robyn/deploys/61c0276ab88ebd0008291261

@ShadowJonathan
Copy link

I would not recommend doing ctx.spawn, websocket messages may be handled out of order due to scheduling.

@sansyrox
Copy link
Member Author

I would not recommend doing ctx.spawn, websocket messages may be handled out of order due to scheduling.

@ShadowJonathan , ctx.spawn is being used for async functions. So, ig out of order execution may work.
Do you have any other suggestions otherwise?

@ShadowJonathan
Copy link

It's mostly so that message B doesn't get processed after message A, I suggest doing the .await on the asyncio coroutine inside the ws object handler function.

@sansyrox
Copy link
Member Author

sansyrox commented Dec 18, 2021

@ShadowJonathan , I tried converting this function(https://github.com/sansyrox/robyn/pull/134/files#diff-0d0da7574dadb5452e2f359138c914a901f38425138f1c2facc37f403b98ab7fR38) to an async function but it is not supported by Actix at the moment :(

Correct me if I am missing something.

@ShadowJonathan
Copy link

Ooooh wait, that explains it some more, yes, then this approach should be fine enough.

Maybe warn users that websocket message handling can be concurrent, and doesn't wait for each handler to complete before spawning the next one.

Or, you could maybe use wait instead.

@sansyrox
Copy link
Member Author

Ooooh wait, that explains it some more, yes, then this approach should be fine enough.

Perfect!

Maybe warn users that websocket message handling can be concurrent, and doesn't wait for each handler to complete before spawning the next one.

This makes sense. If someone wants to have synchronous execution, they can use regular sync functions whereas the async functions can be used when the order does not matter. This would give a more concurrent feel imo.

@ShadowJonathan
Copy link

ShadowJonathan commented Dec 18, 2021

(I recommend trying out that wait function though, it seems plenty more perfect for your usecase)

@sansyrox
Copy link
Member Author

(I recommend trying out that wait function though, it seems plenty more perfect for your usecase)

But won't it block the execution of other calls while the current call is completed?

@ShadowJonathan
Copy link

As far as i see it, it blocks the current task until the asyncio future is completed, so it'll wait until the python async function has handled this message first before it moves on to the next.

Documentation says the actor wont receive any new messages during this time.

@sansyrox
Copy link
Member Author

Documentation says the actor wont receive any new messages during this time.

Hmm.. This is something I didn't want to happen for async functions in WebSockets. Otherwise, I would've used pyo3-asyncio::tokio::run_until_complete method.

I am taking inspiration from expressjs or fastifyjs where async functions may execute out of order to provide a more non-blocking experience.

Sync functions are always executed in order.

@ShadowJonathan
Copy link

ShadowJonathan commented Dec 18, 2021

Just looked up a bunch more documentation from actix, i think it's fine to wait, the actor will receive messages inbetween these handle calls, i think, but it is up to you.

If you're doing spawn, then i really recommend noting this down in documentation.


Edit: Looking some more, "actor cannot receive messages at this time" is probably exactly what you want in this situation, because then the websocket stream will be halted until the python async function has been handled.

Add more support for create, close and message
@sansyrox sansyrox force-pushed the ws-async branch 2 times, most recently from 213cc86 to b40e2e0 Compare December 18, 2021 18:14
@sansyrox
Copy link
Member Author

Edit: Looking some more, "actor cannot receive messages at this time" is probably exactly what you want in this situation, because then the websocket stream will be halted until the python async function has been handled.

I feel maybe I am unable to articulate my point or I am unable to understand why order would be beneficial here. 😅

I'll try again:

I feel that "halting" would block the socket. Which, IMO, should be restricted to synchronous functions. Async functions "should" work out of order without blocking.

@sansyrox
Copy link
Member Author

Now, that I think if the order is really important in async functions. I can just add a param called non_blocking=False

@sansyrox sansyrox changed the title [WIP] Add async support in WS Add async support in WS Dec 20, 2021
@sansyrox sansyrox merged commit 9282a3e into main Dec 20, 2021
@sansyrox sansyrox deleted the ws-async branch December 20, 2021 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Async web socket support
2 participants