Skip to content

Commit

Permalink
don't overload LS with completion resolve requests
Browse files Browse the repository at this point in the history
While moving completion resolve to the event system in helix-editor#9668 we introduced what
is essentially a "DOS attack" on slow LSPs. Completion resolve requests were
made in the render loop and debounced with a timeout. Once the timeout expired
the resolve request was made. The problem is the next frame would immediately
request a new completion resolve request (and mark the old one as obsolete but
because LSP has no notion of cancelation the server would still process it). So
we were in essence sending one completion request to the server every 150ms and
only stopped if the server managed to respond before we rendered a new frame.
This caused overload on slower machines/with slower LS.

In this PR I revamped the resolve handler so that a request is only ever
resolved once. Both by checking if a request is already in-flight and by marking
failed resolve requests as resolved.
  • Loading branch information
pascalkuthe authored and Schuyler Mortimer committed Jul 10, 2024
1 parent 7749aa2 commit 9587b21
Show file tree
Hide file tree
Showing 6 changed files with 194 additions and 120 deletions.
38 changes: 18 additions & 20 deletions helix-lsp/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,16 @@ impl Client {
&self,
params: R::Params,
) -> impl Future<Output = Result<Value>>
where
R::Params: serde::Serialize,
{
self.call_with_ref::<R>(&params)
}

fn call_with_ref<R: lsp::request::Request>(
&self,
params: &R::Params,
) -> impl Future<Output = Result<Value>>
where
R::Params: serde::Serialize,
{
Expand All @@ -405,7 +415,7 @@ impl Client {

fn call_with_timeout<R: lsp::request::Request>(
&self,
params: R::Params,
params: &R::Params,
timeout_secs: u64,
) -> impl Future<Output = Result<Value>>
where
Expand All @@ -414,17 +424,16 @@ impl Client {
let server_tx = self.server_tx.clone();
let id = self.next_request_id();

let params = serde_json::to_value(params);
async move {
use std::time::Duration;
use tokio::time::timeout;

let params = serde_json::to_value(params)?;

let request = jsonrpc::MethodCall {
jsonrpc: Some(jsonrpc::Version::V2),
id: id.clone(),
method: R::METHOD.to_string(),
params: Self::value_into_params(params),
params: Self::value_into_params(params?),
};

let (tx, mut rx) = channel::<Result<Value>>(1);
Expand Down Expand Up @@ -746,7 +755,7 @@ impl Client {
new_uri: url_from_path(new_path)?,
}];
let request = self.call_with_timeout::<lsp::request::WillRenameFiles>(
lsp::RenameFilesParams { files },
&lsp::RenameFilesParams { files },
5,
);

Expand Down Expand Up @@ -1031,21 +1040,10 @@ impl Client {

pub fn resolve_completion_item(
&self,
completion_item: lsp::CompletionItem,
) -> Option<impl Future<Output = Result<lsp::CompletionItem>>> {
let capabilities = self.capabilities.get().unwrap();

// Return early if the server does not support resolving completion items.
match capabilities.completion_provider {
Some(lsp::CompletionOptions {
resolve_provider: Some(true),
..
}) => (),
_ => return None,
}

let res = self.call::<lsp::request::ResolveCompletionItem>(completion_item);
Some(async move { Ok(serde_json::from_value(res.await?)?) })
completion_item: &lsp::CompletionItem,
) -> impl Future<Output = Result<lsp::CompletionItem>> {
let res = self.call_with_ref::<lsp::request::ResolveCompletionItem>(completion_item);
async move { Ok(serde_json::from_value(res.await?)?) }
}

pub fn resolve_code_action(
Expand Down
2 changes: 1 addition & 1 deletion helix-term/src/handlers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use crate::handlers::signature_help::SignatureHelpHandler;
pub use completion::trigger_auto_completion;
pub use helix_view::handlers::Handlers;

mod completion;
pub mod completion;
mod signature_help;

pub fn setup(config: Arc<ArcSwap<Config>>) -> Handlers {
Expand Down
2 changes: 2 additions & 0 deletions helix-term/src/handlers/completion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ use crate::ui::lsp::SignatureHelp;
use crate::ui::{self, CompletionItem, Popup};

use super::Handlers;
pub use resolve::ResolveHandler;
mod resolve;

#[derive(Debug, PartialEq, Eq, Clone, Copy)]
enum TriggerKind {
Expand Down
153 changes: 153 additions & 0 deletions helix-term/src/handlers/completion/resolve.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
use std::sync::Arc;

use helix_lsp::lsp;
use tokio::sync::mpsc::Sender;
use tokio::time::{Duration, Instant};

use helix_event::{send_blocking, AsyncHook, CancelRx};
use helix_view::Editor;

use crate::handlers::completion::CompletionItem;
use crate::job;

/// A hook for resolving incomplete completion items.
///
/// From the [LSP spec](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_completion):
///
/// > If computing full completion items is expensive, servers can additionally provide a
/// > handler for the completion item resolve request. ...
/// > A typical use case is for example: the `textDocument/completion` request doesn't fill
/// > in the `documentation` property for returned completion items since it is expensive
/// > to compute. When the item is selected in the user interface then a
/// > 'completionItem/resolve' request is sent with the selected completion item as a parameter.
/// > The returned completion item should have the documentation property filled in.
pub struct ResolveHandler {
last_request: Option<Arc<CompletionItem>>,
resolver: Sender<ResolveRequest>,
}

impl ResolveHandler {
pub fn new() -> ResolveHandler {
ResolveHandler {
last_request: None,
resolver: ResolveTimeout {
next_request: None,
in_flight: None,
}
.spawn(),
}
}

pub fn ensure_item_resolved(&mut self, editor: &mut Editor, item: &mut CompletionItem) {
if item.resolved {
return;
}
let needs_resolve = item.item.documentation.is_none()
|| item.item.detail.is_none()
|| item.item.additional_text_edits.is_none();
if !needs_resolve {
item.resolved = true;
return;
}
if self.last_request.as_deref().is_some_and(|it| it == item) {
return;
}
let Some(ls) = editor.language_servers.get_by_id(item.provider).cloned() else {
item.resolved = true;
return;
};
if matches!(
ls.capabilities().completion_provider,
Some(lsp::CompletionOptions {
resolve_provider: Some(true),
..
})
) {
let item = Arc::new(item.clone());
self.last_request = Some(item.clone());
send_blocking(&self.resolver, ResolveRequest { item, ls })
} else {
item.resolved = true;
}
}
}

struct ResolveRequest {
item: Arc<CompletionItem>,
ls: Arc<helix_lsp::Client>,
}

#[derive(Default)]
struct ResolveTimeout {
next_request: Option<ResolveRequest>,
in_flight: Option<(helix_event::CancelTx, Arc<CompletionItem>)>,
}

impl AsyncHook for ResolveTimeout {
type Event = ResolveRequest;

fn handle_event(
&mut self,
request: Self::Event,
timeout: Option<tokio::time::Instant>,
) -> Option<tokio::time::Instant> {
if self
.next_request
.as_ref()
.is_some_and(|old_request| old_request.item == request.item)
{
timeout
} else if self
.in_flight
.as_ref()
.is_some_and(|(_, old_request)| old_request.item == request.item.item)
{
self.next_request = None;
None
} else {
self.next_request = Some(request);
Some(Instant::now() + Duration::from_millis(150))
}
}

fn finish_debounce(&mut self) {
let Some(request) = self.next_request.take() else { return };
let (tx, rx) = helix_event::cancelation();
self.in_flight = Some((tx, request.item.clone()));
tokio::spawn(request.execute(rx));
}
}

impl ResolveRequest {
async fn execute(self, cancel: CancelRx) {
let future = self.ls.resolve_completion_item(&self.item.item);
let Some(resolved_item) = helix_event::cancelable_future(future, cancel).await else {
return;
};
job::dispatch(move |_, compositor| {
if let Some(completion) = &mut compositor
.find::<crate::ui::EditorView>()
.unwrap()
.completion
{
let resolved_item = match resolved_item {
Ok(item) => CompletionItem {
item,
resolved: true,
..*self.item
},
Err(err) => {
log::error!("completion resolve request failed: {err}");
// set item to resolved so we don't request it again
// we could also remove it but that oculd be odd ui
let mut item = (*self.item).clone();
item.resolved = true;
item
}
};
completion.replace_item(&self.item, resolved_item);
};
})
.await
}
}
Loading

0 comments on commit 9587b21

Please sign in to comment.