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

Make notification trait methods async and non-blocking #131

Merged
merged 3 commits into from
Mar 3, 2020

Conversation

ebkalderon
Copy link
Owner

Changed

  • Make all LanguageServer notification trait methods async fn and non-blocking.

This pull request makes all LanguageServer notification methods async fn so that we can .await on client responses inside these method bodies, once we implement proper server-to-client communication later on. Instead of spawning these requests with tokio::spawn(), as we currently do in Printer, we could .await them directly inside these methods without blocking the executor.

This also replaces the internal Delegate::delegate_request() and Delegate::delegate_notification() methods with delegate_request!() and the delegate_notification!() macros, respectively, to avoid the nested .awaits and avoiding lifetime woes caused by the lack of proper async closures in the language. Perhaps once async closures get added to Rust later on, we could re-evaluate whether to replace them with dedicated methods again.

Affects #13.

CC @icsaszar

@ebkalderon ebkalderon self-assigned this Feb 29, 2020
@ebkalderon
Copy link
Owner Author

ebkalderon commented Feb 29, 2020

A hypothetical example of making a server-to-client request inside textDocument/didOpen:

async fn did_open(&self, client: &Client, params: DidOpenTextDocumentParams) {
    match client.apply_edit(WorkspaceEdit::default()).await {
        Ok(true) => info!("edit applied!"),
        Ok(false) => info!("edit was not applied!"),
        Err(err) => error!("client returned a JSON-RPC error"),
    }
}

Note that this will require further work on the Printer (likely to be renamed Client), as mentioned in the PR description.

@ebkalderon
Copy link
Owner Author

Feel free to review the changes before I merge at your leisure, @icsaszar!

@ebkalderon ebkalderon force-pushed the make-notifications-async branch from d04cfbb to e2bd899 Compare February 29, 2020 06:17
Copy link
Contributor

@icsaszar icsaszar left a comment

Choose a reason for hiding this comment

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

Very nicely done with the macros, this makes things a lot easier to add and maintain methods.

src/delegate.rs Outdated Show resolved Hide resolved
This commit makes all `LanguageServer` notification methods `async fn`
so that we can `.await` on client responses inside these method bodies,
once we implement proper server-to-client communication later on.
Instead of spawning these requests with `tokio::spawn()`, as we
currently do in `Printer`, we could `.await` them directly inside these
methods without blocking the executor.

This also replaces the internal `Delegate::delegate_request()` and
`Delegate::delegate_notification()` methods with `delegate_request!()`
and the `delegate_notification!()` macros, respectively, to avoid the
nested `.await`s and avoiding lifetime woes caused by the lack of proper
async closures in the language. Perhaps once async closures get added to
Rust later on, we could re-evaluate whether to replace them with
dedicated methods again.
@ebkalderon ebkalderon force-pushed the make-notifications-async branch from 678c241 to af18f46 Compare March 3, 2020 04:38
@ebkalderon ebkalderon merged commit 7e3f2f1 into master Mar 3, 2020
@ebkalderon ebkalderon deleted the make-notifications-async branch March 3, 2020 05:30
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.

2 participants