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

[ws server] async method calls #423

Closed
wants to merge 1 commit into from
Closed

[ws server] async method calls #423

wants to merge 1 commit into from

Conversation

FelipeRosa
Copy link

Closes #422

Hey folks,

I thought of this fix for the problem I reported in #422.

I don't know whether you have preferences of where that tokio::spawn should occur. Since JsonRpcRequest is borrowed, I decided making the tokio::spawn call before parsing the incoming request so data gets moved inside the async block.

Feel free to make any edits you think are appropriate.

@niklasad1 niklasad1 requested a review from maciejhirsz July 23, 2021 21:23
Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks looks good, but I want to hear if @maciejhirsz has any objections on the overhead of tokio::spawn in this context or if we should tokio::spawn inside execute/rpc module instead?!

@maciejhirsz
Copy link
Contributor

I think between this and #408 it would make most sense to just always spawn async calls on tasks. We already need have RpcParams for that so there is no need to allocate a separate buffer for each message (plus using a shared buffer saves us from reallocation issues).

@maciejhirsz
Copy link
Contributor

@FelipeRosa thanks for the PR and bringing our attention to this! I think now that #424 is merged it should have solved your use case so I'll close this PR. Feel free to re-open it or file an issue if there is anything we might have missed.

@FelipeRosa FelipeRosa deleted the wsserver-async-calls branch August 30, 2021 15:12
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.

[ws server] processing calls sequentially
3 participants