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

Avoid calling tokio::spawn internally #180

Closed
ebkalderon opened this issue Apr 29, 2020 · 7 comments · Fixed by #213
Closed

Avoid calling tokio::spawn internally #180

ebkalderon opened this issue Apr 29, 2020 · 7 comments · Fixed by #213
Assignees
Labels
enhancement New feature or request

Comments

@ebkalderon
Copy link
Owner

To help decouple tower-lsp more thoroughly from tokio, we should avoid calling tokio::spawn() internally. Instead, we should look into using executor-agnostic primitives such as join!() and possibly StreamExt::for_each_concurrent() to achieve similar behavior.

@ebkalderon
Copy link
Owner Author

I think that changing Client::log_message(), Client::telemetry(), and Client::send_custom_notification() to async fn and allowing users to .await these methods directly would eliminate most uses of tokio::spawn(). All other uses would be tied to jsonrpc-core and the relevant delegation code.

@ebkalderon
Copy link
Owner Author

With jsonrpc-core completely eliminated from the codebase as of PR #202, the only tokio::spawn() calls remaining exist in the Client methods above. The solution for eliminating these is still the same.

@ebkalderon
Copy link
Owner Author

The WIP code for this task is in the client-notifs-async-fn branch. I'm not happy that the addition of the .await keyword causes rustfmt to overflow all but the shortest of method calls to multiple lines, but it seems to work. Interestingly, this change also requires a bump to the minimum supported Rust version to 1.41.0, as documented in the commit notes. Does this cosmetic change to the API bother anyone?

@silvanshade
Copy link
Contributor

I'm personally not too bothered by the cosmetic changes, fwiw. Thanks for doing all the work to make progress on this!

@ebkalderon
Copy link
Owner Author

Good to know, @darinmorrison! Thanks for the input and for the kind words. Hopefully, the AsyncRead and AsyncWrite traits will be the only things left still tying tower-lsp to tokio soon enough. 😄

@nanne007
Copy link
Contributor

Any update on merging client-notifs-async-fn branch to master?

@ebkalderon
Copy link
Owner Author

@lerencao I was letting the implications of #212 stew in my mind first before merging that PR and opening a new one for this branch. Now that it has been accepted and merged, I'm opening a PR for client-notifs-async-fn as well. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants