Don't retry position relient requests and version resolve data

This commit is contained in:
Lukas Wirth 2024-04-28 16:19:55 +02:00
parent 36c1c77cf9
commit 367b82f68d
8 changed files with 116 additions and 129 deletions

View file

@ -95,45 +95,8 @@ impl RequestDispatcher<'_> {
self
}
/// Dispatches a non-latency-sensitive request onto the thread pool
/// without retrying it if it panics.
pub(crate) fn on_no_retry<R>(
&mut self,
f: fn(GlobalStateSnapshot, R::Params) -> anyhow::Result<R::Result>,
) -> &mut Self
where
R: lsp_types::request::Request + 'static,
R::Params: DeserializeOwned + panic::UnwindSafe + Send + fmt::Debug,
R::Result: Serialize,
{
let (req, params, panic_context) = match self.parse::<R>() {
Some(it) => it,
None => return self,
};
self.global_state.task_pool.handle.spawn(ThreadIntent::Worker, {
let world = self.global_state.snapshot();
move || {
let result = panic::catch_unwind(move || {
let _pctx = stdx::panic_context::enter(panic_context);
f(world, params)
});
match thread_result_to_response::<R>(req.id.clone(), result) {
Ok(response) => Task::Response(response),
Err(_) => Task::Response(lsp_server::Response::new_err(
req.id,
lsp_server::ErrorCode::ContentModified as i32,
"content modified".to_owned(),
)),
}
}
});
self
}
/// Dispatches a non-latency-sensitive request onto the thread pool.
pub(crate) fn on<R>(
pub(crate) fn on<const ALLOW_RETRYING: bool, R>(
&mut self,
f: fn(GlobalStateSnapshot, R::Params) -> anyhow::Result<R::Result>,
) -> &mut Self
@ -142,11 +105,11 @@ impl RequestDispatcher<'_> {
R::Params: DeserializeOwned + panic::UnwindSafe + Send + fmt::Debug,
R::Result: Serialize,
{
self.on_with_thread_intent::<true, R>(ThreadIntent::Worker, f)
self.on_with_thread_intent::<true, ALLOW_RETRYING, R>(ThreadIntent::Worker, f)
}
/// Dispatches a latency-sensitive request onto the thread pool.
pub(crate) fn on_latency_sensitive<R>(
pub(crate) fn on_latency_sensitive<const ALLOW_RETRYING: bool, R>(
&mut self,
f: fn(GlobalStateSnapshot, R::Params) -> anyhow::Result<R::Result>,
) -> &mut Self
@ -155,7 +118,7 @@ impl RequestDispatcher<'_> {
R::Params: DeserializeOwned + panic::UnwindSafe + Send + fmt::Debug,
R::Result: Serialize,
{
self.on_with_thread_intent::<true, R>(ThreadIntent::LatencySensitive, f)
self.on_with_thread_intent::<true, ALLOW_RETRYING, R>(ThreadIntent::LatencySensitive, f)
}
/// Formatting requests should never block on waiting a for task thread to open up, editors will wait
@ -170,7 +133,7 @@ impl RequestDispatcher<'_> {
R::Params: DeserializeOwned + panic::UnwindSafe + Send + fmt::Debug,
R::Result: Serialize,
{
self.on_with_thread_intent::<false, R>(ThreadIntent::LatencySensitive, f)
self.on_with_thread_intent::<false, false, R>(ThreadIntent::LatencySensitive, f)
}
pub(crate) fn finish(&mut self) {
@ -185,7 +148,7 @@ impl RequestDispatcher<'_> {
}
}
fn on_with_thread_intent<const MAIN_POOL: bool, R>(
fn on_with_thread_intent<const MAIN_POOL: bool, const ALLOW_RETRYING: bool, R>(
&mut self,
intent: ThreadIntent,
f: fn(GlobalStateSnapshot, R::Params) -> anyhow::Result<R::Result>,
@ -215,7 +178,12 @@ impl RequestDispatcher<'_> {
});
match thread_result_to_response::<R>(req.id.clone(), result) {
Ok(response) => Task::Response(response),
Err(_) => Task::Retry(req),
Err(_cancelled) if ALLOW_RETRYING => Task::Retry(req),
Err(_cancelled) => Task::Response(lsp_server::Response::new_err(
req.id,
lsp_server::ErrorCode::ContentModified as i32,
"content modified".to_owned(),
)),
}
});

View file

@ -488,6 +488,10 @@ impl GlobalStateSnapshot {
Ok(res)
}
pub(crate) fn file_version(&self, file_id: FileId) -> Option<i32> {
Some(self.mem_docs.get(self.vfs_read().file_path(file_id))?.version)
}
pub(crate) fn url_file_version(&self, url: &Url) -> Option<i32> {
let path = from_proto::vfs_path(url).ok()?;
Some(self.mem_docs.get(&path)?.version)

View file

@ -954,8 +954,13 @@ pub(crate) fn handle_completion(
};
let line_index = snap.file_line_index(position.file_id)?;
let items =
to_proto::completion_items(&snap.config, &line_index, text_document_position, items);
let items = to_proto::completion_items(
&snap.config,
&line_index,
snap.file_version(position.file_id),
text_document_position,
items,
);
let completion_list = lsp_types::CompletionList { is_incomplete: true, items };
Ok(Some(completion_list.into()))
@ -1240,8 +1245,11 @@ pub(crate) fn handle_code_action(
frange,
)?;
for (index, assist) in assists.into_iter().enumerate() {
let resolve_data =
if code_action_resolve_cap { Some((index, params.clone())) } else { None };
let resolve_data = if code_action_resolve_cap {
Some((index, params.clone(), snap.file_version(file_id)))
} else {
None
};
let code_action = to_proto::code_action(&snap, assist, resolve_data)?;
// Check if the client supports the necessary `ResourceOperation`s.
@ -1280,12 +1288,14 @@ pub(crate) fn handle_code_action_resolve(
mut code_action: lsp_ext::CodeAction,
) -> anyhow::Result<lsp_ext::CodeAction> {
let _p = tracing::span!(tracing::Level::INFO, "handle_code_action_resolve").entered();
let params = match code_action.data.take() {
Some(it) => it,
None => return Err(invalid_params_error("code action without data".to_owned()).into()),
let Some(params) = code_action.data.take() else {
return Err(invalid_params_error("code action without data".to_owned()).into());
};
let file_id = from_proto::file_id(&snap, &params.code_action_params.text_document.uri)?;
if snap.file_version(file_id) != params.version {
return Err(invalid_params_error("stale code action".to_owned()).into());
}
let line_index = snap.file_line_index(file_id)?;
let range = from_proto::text_range(&line_index, params.code_action_params.range)?;
let frange = FileRange { file_id, range };
@ -1411,12 +1421,11 @@ pub(crate) fn handle_code_lens(
pub(crate) fn handle_code_lens_resolve(
snap: GlobalStateSnapshot,
code_lens: CodeLens,
mut code_lens: CodeLens,
) -> anyhow::Result<CodeLens> {
if code_lens.data.is_none() {
return Ok(code_lens);
}
let Some(annotation) = from_proto::annotation(&snap, code_lens.clone())? else {
let Some(data) = code_lens.data.take() else { return Ok(code_lens) };
let resolve = serde_json::from_value::<lsp_ext::CodeLensResolveData>(data)?;
let Some(annotation) = from_proto::annotation(&snap, code_lens.range, resolve)? else {
return Ok(code_lens);
};
let annotation = snap.analysis.resolve_annotation(annotation)?;
@ -1522,8 +1531,12 @@ pub(crate) fn handle_inlay_hints_resolve(
let Some(data) = original_hint.data.take() else { return Ok(original_hint) };
let resolve_data: lsp_ext::InlayHintResolveData = serde_json::from_value(data)?;
let Some(hash) = resolve_data.hash.parse().ok() else { return Ok(original_hint) };
let file_id = FileId::from_raw(resolve_data.file_id);
if resolve_data.version != snap.file_version(file_id) {
tracing::warn!("Inlay hint resolve data is outdated");
return Ok(original_hint);
}
let Some(hash) = resolve_data.hash.parse().ok() else { return Ok(original_hint) };
anyhow::ensure!(snap.file_exists(file_id), "Invalid LSP resolve data");
let line_index = snap.file_line_index(file_id)?;

View file

@ -561,6 +561,7 @@ pub struct CodeAction {
pub struct CodeActionData {
pub code_action_params: lsp_types::CodeActionParams,
pub id: String,
pub version: Option<i32>,
}
#[derive(Debug, Eq, PartialEq, Clone, Default, Deserialize, Serialize)]
@ -802,6 +803,7 @@ impl Request for OnTypeFormatting {
pub struct CompletionResolveData {
pub position: lsp_types::TextDocumentPositionParams,
pub imports: Vec<CompletionImport>,
pub version: Option<i32>,
}
#[derive(Debug, Serialize, Deserialize)]
@ -809,6 +811,7 @@ pub struct InlayHintResolveData {
pub file_id: u32,
// This is a string instead of a u64 as javascript can't represent u64 fully
pub hash: String,
pub version: Option<i32>,
}
#[derive(Debug, Serialize, Deserialize)]

View file

@ -9,10 +9,8 @@ use syntax::{TextRange, TextSize};
use vfs::AbsPathBuf;
use crate::{
from_json,
global_state::GlobalStateSnapshot,
line_index::{LineIndex, PositionEncoding},
lsp::utils::invalid_params_error,
lsp_ext,
};
@ -105,16 +103,13 @@ pub(crate) fn assist_kind(kind: lsp_types::CodeActionKind) -> Option<AssistKind>
pub(crate) fn annotation(
snap: &GlobalStateSnapshot,
code_lens: lsp_types::CodeLens,
range: lsp_types::Range,
data: lsp_ext::CodeLensResolveData,
) -> anyhow::Result<Option<Annotation>> {
let data =
code_lens.data.ok_or_else(|| invalid_params_error("code lens without data".to_owned()))?;
let resolve = from_json::<lsp_ext::CodeLensResolveData>("CodeLensResolveData", &data)?;
match resolve.kind {
match data.kind {
lsp_ext::CodeLensResolveDataKind::Impls(params) => {
if snap.url_file_version(&params.text_document_position_params.text_document.uri)
!= Some(resolve.version)
!= Some(data.version)
{
return Ok(None);
}
@ -123,19 +118,19 @@ pub(crate) fn annotation(
let line_index = snap.file_line_index(file_id)?;
Ok(Annotation {
range: text_range(&line_index, code_lens.range)?,
range: text_range(&line_index, range)?,
kind: AnnotationKind::HasImpls { pos, data: None },
})
}
lsp_ext::CodeLensResolveDataKind::References(params) => {
if snap.url_file_version(&params.text_document.uri) != Some(resolve.version) {
if snap.url_file_version(&params.text_document.uri) != Some(data.version) {
return Ok(None);
}
let pos @ FilePosition { file_id, .. } = file_position(snap, params)?;
let line_index = snap.file_line_index(file_id)?;
Ok(Annotation {
range: text_range(&line_index, code_lens.range)?,
range: text_range(&line_index, range)?,
kind: AnnotationKind::HasReferences { pos, data: None },
})
}

View file

@ -225,13 +225,14 @@ pub(crate) fn snippet_text_edit_vec(
pub(crate) fn completion_items(
config: &Config,
line_index: &LineIndex,
version: Option<i32>,
tdpp: lsp_types::TextDocumentPositionParams,
items: Vec<CompletionItem>,
) -> Vec<lsp_types::CompletionItem> {
let max_relevance = items.iter().map(|it| it.relevance.score()).max().unwrap_or_default();
let mut res = Vec::with_capacity(items.len());
for item in items {
completion_item(&mut res, config, line_index, &tdpp, max_relevance, item);
completion_item(&mut res, config, line_index, version, &tdpp, max_relevance, item);
}
if let Some(limit) = config.completion(None).limit {
@ -246,6 +247,7 @@ fn completion_item(
acc: &mut Vec<lsp_types::CompletionItem>,
config: &Config,
line_index: &LineIndex,
version: Option<i32>,
tdpp: &lsp_types::TextDocumentPositionParams,
max_relevance: u32,
item: CompletionItem,
@ -328,7 +330,7 @@ fn completion_item(
})
.collect::<Vec<_>>();
if !imports.is_empty() {
let data = lsp_ext::CompletionResolveData { position: tdpp.clone(), imports };
let data = lsp_ext::CompletionResolveData { position: tdpp.clone(), imports, version };
lsp_item.data = Some(to_value(data).unwrap());
}
}
@ -483,6 +485,7 @@ pub(crate) fn inlay_hint(
to_value(lsp_ext::InlayHintResolveData {
file_id: file_id.index(),
hash: hash.to_string(),
version: snap.file_version(file_id),
})
.unwrap(),
),
@ -1318,7 +1321,7 @@ pub(crate) fn code_action_kind(kind: AssistKind) -> lsp_types::CodeActionKind {
pub(crate) fn code_action(
snap: &GlobalStateSnapshot,
assist: Assist,
resolve_data: Option<(usize, lsp_types::CodeActionParams)>,
resolve_data: Option<(usize, lsp_types::CodeActionParams, Option<i32>)>,
) -> Cancellable<lsp_ext::CodeAction> {
let mut res = lsp_ext::CodeAction {
title: assist.label.to_string(),
@ -1336,10 +1339,11 @@ pub(crate) fn code_action(
match (assist.source_change, resolve_data) {
(Some(it), _) => res.edit = Some(snippet_workspace_edit(snap, it)?),
(None, Some((index, code_action_params))) => {
(None, Some((index, code_action_params, version))) => {
res.data = Some(lsp_ext::CodeActionData {
id: format!("{}:{}:{index}", assist.id.0, assist.id.1.name()),
code_action_params,
version,
});
}
(None, None) => {

View file

@ -901,6 +901,10 @@ impl GlobalState {
use crate::handlers::request as handlers;
use lsp_types::request as lsp_request;
const RETRY: bool = true;
const NO_RETRY: bool = false;
#[rustfmt::skip]
dispatcher
// Request handlers that must run on the main thread
// because they mutate GlobalState:
@ -926,62 +930,58 @@ impl GlobalState {
// analysis on the main thread because that would block other
// requests. Instead, we run these request handlers on higher priority
// threads in the threadpool.
.on_latency_sensitive::<lsp_request::Completion>(handlers::handle_completion)
.on_latency_sensitive::<lsp_request::ResolveCompletionItem>(
handlers::handle_completion_resolve,
)
.on_latency_sensitive::<lsp_request::SemanticTokensFullRequest>(
handlers::handle_semantic_tokens_full,
)
.on_latency_sensitive::<lsp_request::SemanticTokensFullDeltaRequest>(
handlers::handle_semantic_tokens_full_delta,
)
.on_latency_sensitive::<lsp_request::SemanticTokensRangeRequest>(
handlers::handle_semantic_tokens_range,
)
// FIXME: Retrying can make the result of this stale?
.on_latency_sensitive::<RETRY, lsp_request::Completion>(handlers::handle_completion)
// FIXME: Retrying can make the result of this stale
.on_latency_sensitive::<RETRY, lsp_request::ResolveCompletionItem>(handlers::handle_completion_resolve)
.on_latency_sensitive::<RETRY, lsp_request::SemanticTokensFullRequest>(handlers::handle_semantic_tokens_full)
.on_latency_sensitive::<RETRY, lsp_request::SemanticTokensFullDeltaRequest>(handlers::handle_semantic_tokens_full_delta)
.on_latency_sensitive::<NO_RETRY, lsp_request::SemanticTokensRangeRequest>(handlers::handle_semantic_tokens_range)
// FIXME: Some of these NO_RETRY could be retries if the file they are interested didn't change.
// All other request handlers
.on::<lsp_ext::FetchDependencyList>(handlers::fetch_dependency_list)
.on::<lsp_ext::AnalyzerStatus>(handlers::handle_analyzer_status)
.on::<lsp_ext::SyntaxTree>(handlers::handle_syntax_tree)
.on::<lsp_ext::ViewHir>(handlers::handle_view_hir)
.on::<lsp_ext::ViewMir>(handlers::handle_view_mir)
.on::<lsp_ext::InterpretFunction>(handlers::handle_interpret_function)
.on::<lsp_ext::ViewFileText>(handlers::handle_view_file_text)
.on::<lsp_ext::ViewCrateGraph>(handlers::handle_view_crate_graph)
.on::<lsp_ext::ViewItemTree>(handlers::handle_view_item_tree)
.on::<lsp_ext::DiscoverTest>(handlers::handle_discover_test)
.on::<lsp_ext::ExpandMacro>(handlers::handle_expand_macro)
.on::<lsp_ext::ParentModule>(handlers::handle_parent_module)
.on::<lsp_ext::Runnables>(handlers::handle_runnables)
.on::<lsp_ext::RelatedTests>(handlers::handle_related_tests)
.on::<lsp_ext::CodeActionRequest>(handlers::handle_code_action)
.on::<lsp_ext::CodeActionResolveRequest>(handlers::handle_code_action_resolve)
.on::<lsp_ext::HoverRequest>(handlers::handle_hover)
.on::<lsp_ext::ExternalDocs>(handlers::handle_open_docs)
.on::<lsp_ext::OpenCargoToml>(handlers::handle_open_cargo_toml)
.on::<lsp_ext::MoveItem>(handlers::handle_move_item)
.on::<lsp_ext::WorkspaceSymbol>(handlers::handle_workspace_symbol)
.on::<lsp_request::DocumentSymbolRequest>(handlers::handle_document_symbol)
.on::<lsp_request::GotoDefinition>(handlers::handle_goto_definition)
.on::<lsp_request::GotoDeclaration>(handlers::handle_goto_declaration)
.on::<lsp_request::GotoImplementation>(handlers::handle_goto_implementation)
.on::<lsp_request::GotoTypeDefinition>(handlers::handle_goto_type_definition)
.on_no_retry::<lsp_request::InlayHintRequest>(handlers::handle_inlay_hints)
.on::<lsp_request::InlayHintResolveRequest>(handlers::handle_inlay_hints_resolve)
.on::<lsp_request::CodeLensRequest>(handlers::handle_code_lens)
.on::<lsp_request::CodeLensResolve>(handlers::handle_code_lens_resolve)
.on::<lsp_request::FoldingRangeRequest>(handlers::handle_folding_range)
.on::<lsp_request::SignatureHelpRequest>(handlers::handle_signature_help)
.on::<lsp_request::PrepareRenameRequest>(handlers::handle_prepare_rename)
.on::<lsp_request::Rename>(handlers::handle_rename)
.on::<lsp_request::References>(handlers::handle_references)
.on::<lsp_request::DocumentHighlightRequest>(handlers::handle_document_highlight)
.on::<lsp_request::CallHierarchyPrepare>(handlers::handle_call_hierarchy_prepare)
.on::<lsp_request::CallHierarchyIncomingCalls>(handlers::handle_call_hierarchy_incoming)
.on::<lsp_request::CallHierarchyOutgoingCalls>(handlers::handle_call_hierarchy_outgoing)
.on::<lsp_request::WillRenameFiles>(handlers::handle_will_rename_files)
.on::<lsp_ext::Ssr>(handlers::handle_ssr)
.on::<lsp_ext::ViewRecursiveMemoryLayout>(handlers::handle_view_recursive_memory_layout)
.on::<RETRY, lsp_request::DocumentSymbolRequest>(handlers::handle_document_symbol)
.on::<RETRY, lsp_request::FoldingRangeRequest>(handlers::handle_folding_range)
.on::<NO_RETRY, lsp_request::SignatureHelpRequest>(handlers::handle_signature_help)
.on::<RETRY, lsp_request::WillRenameFiles>(handlers::handle_will_rename_files)
.on::<NO_RETRY, lsp_request::GotoDefinition>(handlers::handle_goto_definition)
.on::<NO_RETRY, lsp_request::GotoDeclaration>(handlers::handle_goto_declaration)
.on::<NO_RETRY, lsp_request::GotoImplementation>(handlers::handle_goto_implementation)
.on::<NO_RETRY, lsp_request::GotoTypeDefinition>(handlers::handle_goto_type_definition)
.on::<NO_RETRY, lsp_request::InlayHintRequest>(handlers::handle_inlay_hints)
.on::<RETRY, lsp_request::InlayHintResolveRequest>(handlers::handle_inlay_hints_resolve)
.on::<NO_RETRY, lsp_request::CodeLensRequest>(handlers::handle_code_lens)
.on::<RETRY, lsp_request::CodeLensResolve>(handlers::handle_code_lens_resolve)
.on::<NO_RETRY, lsp_request::PrepareRenameRequest>(handlers::handle_prepare_rename)
.on::<NO_RETRY, lsp_request::Rename>(handlers::handle_rename)
.on::<NO_RETRY, lsp_request::References>(handlers::handle_references)
.on::<NO_RETRY, lsp_request::DocumentHighlightRequest>(handlers::handle_document_highlight)
.on::<NO_RETRY, lsp_request::CallHierarchyPrepare>(handlers::handle_call_hierarchy_prepare)
.on::<NO_RETRY, lsp_request::CallHierarchyIncomingCalls>(handlers::handle_call_hierarchy_incoming)
.on::<NO_RETRY, lsp_request::CallHierarchyOutgoingCalls>(handlers::handle_call_hierarchy_outgoing)
// All other request handlers (lsp extension)
.on::<RETRY, lsp_ext::FetchDependencyList>(handlers::fetch_dependency_list)
.on::<RETRY, lsp_ext::AnalyzerStatus>(handlers::handle_analyzer_status)
.on::<RETRY, lsp_ext::ViewFileText>(handlers::handle_view_file_text)
.on::<RETRY, lsp_ext::ViewCrateGraph>(handlers::handle_view_crate_graph)
.on::<RETRY, lsp_ext::ViewItemTree>(handlers::handle_view_item_tree)
.on::<RETRY, lsp_ext::DiscoverTest>(handlers::handle_discover_test)
.on::<RETRY, lsp_ext::WorkspaceSymbol>(handlers::handle_workspace_symbol)
.on::<NO_RETRY, lsp_ext::Ssr>(handlers::handle_ssr)
.on::<NO_RETRY, lsp_ext::ViewRecursiveMemoryLayout>(handlers::handle_view_recursive_memory_layout)
.on::<NO_RETRY, lsp_ext::SyntaxTree>(handlers::handle_syntax_tree)
.on::<NO_RETRY, lsp_ext::ViewHir>(handlers::handle_view_hir)
.on::<NO_RETRY, lsp_ext::ViewMir>(handlers::handle_view_mir)
.on::<NO_RETRY, lsp_ext::InterpretFunction>(handlers::handle_interpret_function)
.on::<NO_RETRY, lsp_ext::ExpandMacro>(handlers::handle_expand_macro)
.on::<NO_RETRY, lsp_ext::ParentModule>(handlers::handle_parent_module)
.on::<NO_RETRY, lsp_ext::Runnables>(handlers::handle_runnables)
.on::<NO_RETRY, lsp_ext::RelatedTests>(handlers::handle_related_tests)
.on::<NO_RETRY, lsp_ext::CodeActionRequest>(handlers::handle_code_action)
.on::<RETRY, lsp_ext::CodeActionResolveRequest>(handlers::handle_code_action_resolve)
.on::<NO_RETRY, lsp_ext::HoverRequest>(handlers::handle_hover)
.on::<NO_RETRY, lsp_ext::ExternalDocs>(handlers::handle_open_docs)
.on::<NO_RETRY, lsp_ext::OpenCargoToml>(handlers::handle_open_cargo_toml)
.on::<NO_RETRY, lsp_ext::MoveItem>(handlers::handle_move_item)
.finish();
}

View file

@ -1,5 +1,5 @@
<!---
lsp/ext.rs hash: d8e2aa65fdb48e48
lsp/ext.rs hash: a39009c351009d16
If you need to change the above hash to make the test pass, please check if you
need to adjust this doc as well and ping this issue: