From 38ee845b052d306b37863526098613483000cc16 Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Sun, 21 Apr 2024 16:14:39 +0200 Subject: [PATCH] don't overload LS with completion resolve requests While moving completion resolve to the event system in #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. --- helix-lsp/src/client.rs | 38 +++-- helix-term/src/handlers.rs | 2 +- helix-term/src/handlers/completion.rs | 2 + helix-term/src/handlers/completion/resolve.rs | 153 ++++++++++++++++++ helix-term/src/ui/completion.rs | 115 +++---------- helix-term/src/ui/menu.rs | 4 +- 6 files changed, 194 insertions(+), 120 deletions(-) create mode 100644 helix-term/src/handlers/completion/resolve.rs diff --git a/helix-lsp/src/client.rs b/helix-lsp/src/client.rs index 79d8adb3..254625a3 100644 --- a/helix-lsp/src/client.rs +++ b/helix-lsp/src/client.rs @@ -397,6 +397,16 @@ impl Client { &self, params: R::Params, ) -> impl Future> + where + R::Params: serde::Serialize, + { + self.call_with_ref::(¶ms) + } + + fn call_with_ref( + &self, + params: &R::Params, + ) -> impl Future> where R::Params: serde::Serialize, { @@ -405,7 +415,7 @@ impl Client { fn call_with_timeout( &self, - params: R::Params, + params: &R::Params, timeout_secs: u64, ) -> impl Future> where @@ -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::>(1); @@ -741,7 +750,7 @@ impl Client { new_uri: url_from_path(new_path)?, }]; let request = self.call_with_timeout::( - lsp::RenameFilesParams { files }, + &lsp::RenameFilesParams { files }, 5, ); @@ -1026,21 +1035,10 @@ impl Client { pub fn resolve_completion_item( &self, - completion_item: lsp::CompletionItem, - ) -> Option>> { - 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::(completion_item); - Some(async move { Ok(serde_json::from_value(res.await?)?) }) + completion_item: &lsp::CompletionItem, + ) -> impl Future> { + let res = self.call_with_ref::(completion_item); + async move { Ok(serde_json::from_value(res.await?)?) } } pub fn resolve_code_action( diff --git a/helix-term/src/handlers.rs b/helix-term/src/handlers.rs index 1b7d9b8c..d45809d3 100644 --- a/helix-term/src/handlers.rs +++ b/helix-term/src/handlers.rs @@ -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>) -> Handlers { diff --git a/helix-term/src/handlers/completion.rs b/helix-term/src/handlers/completion.rs index 8bd85ef6..68956c85 100644 --- a/helix-term/src/handlers/completion.rs +++ b/helix-term/src/handlers/completion.rs @@ -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 { diff --git a/helix-term/src/handlers/completion/resolve.rs b/helix-term/src/handlers/completion/resolve.rs new file mode 100644 index 00000000..fb5179e1 --- /dev/null +++ b/helix-term/src/handlers/completion/resolve.rs @@ -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>, + resolver: Sender, +} + +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, + ls: Arc, +} + +#[derive(Default)] +struct ResolveTimeout { + next_request: Option, + in_flight: Option<(helix_event::CancelTx, Arc)>, +} + +impl AsyncHook for ResolveTimeout { + type Event = ResolveRequest; + + fn handle_event( + &mut self, + request: Self::Event, + timeout: Option, + ) -> Option { + 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::() + .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 + } +} diff --git a/helix-term/src/ui/completion.rs b/helix-term/src/ui/completion.rs index 2b3437e1..7a08c90c 100644 --- a/helix-term/src/ui/completion.rs +++ b/helix-term/src/ui/completion.rs @@ -1,9 +1,7 @@ use crate::{ compositor::{Component, Context, Event, EventResult}, - handlers::trigger_auto_completion, - job, + handlers::{completion::ResolveHandler, trigger_auto_completion}, }; -use helix_event::AsyncHook; use helix_view::{ document::SavePoint, editor::CompleteAction, @@ -12,17 +10,16 @@ use helix_view::{ theme::{Modifier, Style}, ViewId, }; -use tokio::time::Instant; use tui::{buffer::Buffer as Surface, text::Span}; -use std::{borrow::Cow, sync::Arc, time::Duration}; +use std::{borrow::Cow, sync::Arc}; use helix_core::{chars, Change, Transaction}; use helix_view::{graphics::Rect, Document, Editor}; use crate::ui::{menu, Markdown, Menu, Popup, PromptEvent}; -use helix_lsp::{lsp, util, OffsetEncoding}; +use helix_lsp::{lsp, util, LanguageServerId, OffsetEncoding}; impl menu::Item for CompletionItem { type Data = (); @@ -104,7 +101,7 @@ pub struct Completion { #[allow(dead_code)] trigger_offset: usize, filter: String, - resolve_handler: tokio::sync::mpsc::Sender, + resolve_handler: ResolveHandler, } impl Completion { @@ -365,7 +362,7 @@ impl Completion { // TODO: expand nucleo api to allow moving straight to a Utf32String here // and avoid allocation during matching filter: String::from(fragment), - resolve_handler: ResolveHandler::default().spawn(), + resolve_handler: ResolveHandler::new(), }; // need to recompute immediately in case start_offset != trigger_offset @@ -383,7 +380,16 @@ impl Completion { language_server: &helix_lsp::Client, completion_item: lsp::CompletionItem, ) -> Option { - let future = language_server.resolve_completion_item(completion_item)?; + if !matches!( + language_server.capabilities().completion_provider, + Some(lsp::CompletionOptions { + resolve_provider: Some(true), + .. + }) + ) { + return None; + } + let future = language_server.resolve_completion_item(&completion_item); let response = helix_lsp::block_on(future); match response { Ok(item) => Some(item), @@ -416,7 +422,7 @@ impl Completion { self.popup.contents().is_empty() } - fn replace_item(&mut self, old_item: CompletionItem, new_item: CompletionItem) { + pub fn replace_item(&mut self, old_item: &CompletionItem, new_item: CompletionItem) { self.popup.contents_mut().replace_option(old_item, new_item); } @@ -438,12 +444,12 @@ impl Component for Completion { self.popup.render(area, surface, cx); // if we have a selection, render a markdown popup on top/below with info - let option = match self.popup.contents().selection() { + let option = match self.popup.contents_mut().selection_mut() { Some(option) => option, None => return, }; if !option.resolved { - helix_event::send_blocking(&self.resolve_handler, option.clone()); + self.resolve_handler.ensure_item_resolved(cx.editor, option); } // need to render: // option.detail @@ -541,88 +547,3 @@ impl Component for Completion { markdown_doc.render(doc_area, surface, cx); } } - -/// 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. -#[derive(Debug, Default)] -struct ResolveHandler { - trigger: Option, - request: Option, -} - -impl AsyncHook for ResolveHandler { - type Event = CompletionItem; - - fn handle_event( - &mut self, - item: Self::Event, - timeout: Option, - ) -> Option { - if self - .trigger - .as_ref() - .is_some_and(|trigger| trigger == &item) - { - timeout - } else { - self.trigger = Some(item); - self.request = None; - Some(Instant::now() + Duration::from_millis(150)) - } - } - - fn finish_debounce(&mut self) { - let Some(item) = self.trigger.take() else { return }; - let (tx, rx) = helix_event::cancelation(); - self.request = Some(tx); - job::dispatch_blocking(move |editor, _| resolve_completion_item(editor, item, rx)) - } -} - -fn resolve_completion_item( - editor: &mut Editor, - item: CompletionItem, - cancel: helix_event::CancelRx, -) { - let Some(language_server) = editor.language_server_by_id(item.language_server_id) else { - return; - }; - - let Some(future) = language_server.resolve_completion_item(item.item.clone()) else { - return; - }; - - tokio::spawn(async move { - match helix_event::cancelable_future(future, cancel).await { - Some(Ok(resolved_item)) => { - job::dispatch(move |_, compositor| { - if let Some(completion) = &mut compositor - .find::() - .unwrap() - .completion - { - let resolved_item = CompletionItem { - item: resolved_item, - language_server_id: item.language_server_id, - resolved: true, - }; - - completion.replace_item(item, resolved_item); - }; - }) - .await - } - Some(Err(err)) => log::error!("completion resolve request failed: {err}"), - None => (), - } - }); -} diff --git a/helix-term/src/ui/menu.rs b/helix-term/src/ui/menu.rs index c0e60b33..f9f038e7 100644 --- a/helix-term/src/ui/menu.rs +++ b/helix-term/src/ui/menu.rs @@ -241,9 +241,9 @@ impl Menu { } impl Menu { - pub fn replace_option(&mut self, old_option: T, new_option: T) { + pub fn replace_option(&mut self, old_option: &T, new_option: T) { for option in &mut self.options { - if old_option == *option { + if old_option == option { *option = new_option; break; }