From e7086a35b56d75eb29c746c877794d270e8eaac5 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Wed, 7 Aug 2024 16:35:42 +0200 Subject: [PATCH 1/2] Slightly quieter fatal panics --- .../crates/rust-analyzer/src/main_loop.rs | 30 +++++++++++-------- .../crates/vfs-notify/src/lib.rs | 5 +++- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/src/tools/rust-analyzer/crates/rust-analyzer/src/main_loop.rs b/src/tools/rust-analyzer/crates/rust-analyzer/src/main_loop.rs index 42eb6d2e79c2..75635d1c2884 100644 --- a/src/tools/rust-analyzer/crates/rust-analyzer/src/main_loop.rs +++ b/src/tools/rust-analyzer/crates/rust-analyzer/src/main_loop.rs @@ -179,7 +179,10 @@ impl GlobalState { } } - while let Some(event) = self.next_event(&inbox) { + while let Ok(event) = self.next_event(&inbox) { + let Some(event) = event else { + anyhow::bail!("client exited without proper shutdown sequence"); + }; if matches!( &event, Event::Lsp(lsp_server::Message::Notification(Notification { method, .. })) @@ -190,7 +193,7 @@ impl GlobalState { self.handle_event(event)?; } - anyhow::bail!("client exited without proper shutdown sequence") + Err(anyhow::anyhow!("A receiver has been dropped, something panicked!")) } fn register_did_save_capability(&mut self, additional_patterns: impl Iterator) { @@ -237,37 +240,40 @@ impl GlobalState { ); } - fn next_event(&self, inbox: &Receiver) -> Option { + fn next_event( + &self, + inbox: &Receiver, + ) -> Result, crossbeam_channel::RecvError> { select! { recv(inbox) -> msg => - msg.ok().map(Event::Lsp), + return Ok(msg.ok().map(Event::Lsp)), recv(self.task_pool.receiver) -> task => - Some(Event::Task(task.unwrap())), + task.map(Event::Task), recv(self.deferred_task_queue.receiver) -> task => - Some(Event::QueuedTask(task.unwrap())), + task.map(Event::QueuedTask), recv(self.fmt_pool.receiver) -> task => - Some(Event::Task(task.unwrap())), + task.map(Event::Task), recv(self.loader.receiver) -> task => - Some(Event::Vfs(task.unwrap())), + task.map(Event::Vfs), recv(self.flycheck_receiver) -> task => - Some(Event::Flycheck(task.unwrap())), + task.map(Event::Flycheck), recv(self.test_run_receiver) -> task => - Some(Event::TestResult(task.unwrap())), + task.map(Event::TestResult), recv(self.discover_receiver) -> task => - Some(Event::DiscoverProject(task.unwrap())), + task.map(Event::DiscoverProject), } + .map(Some) } fn handle_event(&mut self, event: Event) -> anyhow::Result<()> { let loop_start = Instant::now(); - // NOTE: don't count blocking select! call as a loop-turn time let _p = tracing::info_span!("GlobalState::handle_event", event = %event).entered(); let event_dbg_msg = format!("{event:?}"); diff --git a/src/tools/rust-analyzer/crates/vfs-notify/src/lib.rs b/src/tools/rust-analyzer/crates/vfs-notify/src/lib.rs index d0d3a844465d..57e83ac0a89b 100644 --- a/src/tools/rust-analyzer/crates/vfs-notify/src/lib.rs +++ b/src/tools/rust-analyzer/crates/vfs-notify/src/lib.rs @@ -103,7 +103,10 @@ impl NotifyActor { let (watcher_sender, watcher_receiver) = unbounded(); let watcher = log_notify_error(RecommendedWatcher::new( move |event| { - watcher_sender.send(event).unwrap(); + // we don't care about the error. If sending fails that usually + // means we were dropped, so unwrapping will just add to the + // panic noise. + _ = watcher_sender.send(event); }, Config::default(), )); From 3e4632d99363615d585bae18511c08d3cfc49fad Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Wed, 7 Aug 2024 16:43:51 +0200 Subject: [PATCH 2/2] Offload diagnostics serialization to the task pool --- .../crates/rust-analyzer/src/global_state.rs | 43 +++++++++++++++++++ .../crates/rust-analyzer/src/main_loop.rs | 37 +++------------- 2 files changed, 48 insertions(+), 32 deletions(-) diff --git a/src/tools/rust-analyzer/crates/rust-analyzer/src/global_state.rs b/src/tools/rust-analyzer/crates/rust-analyzer/src/global_state.rs index 12146c8ce0a8..9d064ed12260 100644 --- a/src/tools/rust-analyzer/crates/rust-analyzer/src/global_state.rs +++ b/src/tools/rust-analyzer/crates/rust-analyzer/src/global_state.rs @@ -560,6 +560,49 @@ impl GlobalState { fn send(&self, message: lsp_server::Message) { self.sender.send(message).unwrap() } + + pub(crate) fn publish_diagnostics( + &mut self, + uri: Url, + version: Option, + mut diagnostics: Vec, + ) { + // We put this on a separate thread to avoid blocking the main thread with serialization work + self.task_pool.handle.spawn_with_sender(stdx::thread::ThreadIntent::Worker, { + let sender = self.sender.clone(); + move |_| { + // VSCode assumes diagnostic messages to be non-empty strings, so we need to patch + // empty diagnostics. Neither the docs of VSCode nor the LSP spec say whether + // diagnostic messages are actually allowed to be empty or not and patching this + // in the VSCode client does not work as the assertion happens in the protocol + // conversion. So this hack is here to stay, and will be considered a hack + // until the LSP decides to state that empty messages are allowed. + + // See https://github.com/rust-lang/rust-analyzer/issues/11404 + // See https://github.com/rust-lang/rust-analyzer/issues/13130 + let patch_empty = |message: &mut String| { + if message.is_empty() { + " ".clone_into(message); + } + }; + + for d in &mut diagnostics { + patch_empty(&mut d.message); + if let Some(dri) = &mut d.related_information { + for dri in dri { + patch_empty(&mut dri.message); + } + } + } + + let not = lsp_server::Notification::new( + ::METHOD.to_owned(), + lsp_types::PublishDiagnosticsParams { uri, diagnostics, version }, + ); + _ = sender.send(not.into()); + } + }); + } } impl Drop for GlobalState { diff --git a/src/tools/rust-analyzer/crates/rust-analyzer/src/main_loop.rs b/src/tools/rust-analyzer/crates/rust-analyzer/src/main_loop.rs index 75635d1c2884..54f718479f05 100644 --- a/src/tools/rust-analyzer/crates/rust-analyzer/src/main_loop.rs +++ b/src/tools/rust-analyzer/crates/rust-analyzer/src/main_loop.rs @@ -440,40 +440,13 @@ impl GlobalState { if let Some(diagnostic_changes) = self.diagnostics.take_changes() { for file_id in diagnostic_changes { let uri = file_id_to_url(&self.vfs.read().0, file_id); - let mut diagnostics = - self.diagnostics.diagnostics_for(file_id).cloned().collect::>(); - - // VSCode assumes diagnostic messages to be non-empty strings, so we need to patch - // empty diagnostics. Neither the docs of VSCode nor the LSP spec say whether - // diagnostic messages are actually allowed to be empty or not and patching this - // in the VSCode client does not work as the assertion happens in the protocol - // conversion. So this hack is here to stay, and will be considered a hack - // until the LSP decides to state that empty messages are allowed. - - // See https://github.com/rust-lang/rust-analyzer/issues/11404 - // See https://github.com/rust-lang/rust-analyzer/issues/13130 - let patch_empty = |message: &mut String| { - if message.is_empty() { - " ".clone_into(message); - } - }; - - for d in &mut diagnostics { - patch_empty(&mut d.message); - if let Some(dri) = &mut d.related_information { - for dri in dri { - patch_empty(&mut dri.message); - } - } - } - let version = from_proto::vfs_path(&uri) - .map(|path| self.mem_docs.get(&path).map(|it| it.version)) - .unwrap_or_default(); + .ok() + .and_then(|path| self.mem_docs.get(&path).map(|it| it.version)); - self.send_notification::( - lsp_types::PublishDiagnosticsParams { uri, diagnostics, version }, - ); + let diagnostics = + self.diagnostics.diagnostics_for(file_id).cloned().collect::>(); + self.publish_diagnostics(uri, version, diagnostics); } }