From ca1d0a6e5958598d299cf1cc7936bb6330760903 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Tue, 28 Jun 2022 09:44:43 +0200 Subject: [PATCH] Ignore tokens that were not created via `WorkDoneProgressCreate` With the new version of rust-analyzer, we were seeing stray `WorkDoneProgress::End` messages that create an imbalance in the `pending_diagnostic_updates` that never resolves. This was causing the diagnostic status bar item to never update because we wouldn't emit `DiskBasedDiagnosticsStarted` nor `DiskBasedDiagnosticsFinished`. This commit fixes the above situation by only acknowledging progress report for tokens that have explicitly been created via the `WorkDoneProgressCreate` request, as stated by the protocol. In addition to that, we are replacing the `pending_diagnostic_updates: isize` with a `has_pending_diagnostic_updates: bool`. We added it at some point to prevent a similar issue where we would observe begin/end reports in a seemingly random order, which would cause us to permanently display a `checking...` message in the status bar. I believe this commit fixes that as well because the `isize` was just a less general solution for the same underlying issue. As the protocol states: "the token provided in the create request should only be used once (e.g. only one begin, many report and one end notification should be sent to it)." --- crates/collab/src/integration_tests.rs | 1 + crates/lsp/src/lsp.rs | 20 +++++- crates/project/src/project.rs | 91 +++++++++++++++----------- 3 files changed, 72 insertions(+), 40 deletions(-) diff --git a/crates/collab/src/integration_tests.rs b/crates/collab/src/integration_tests.rs index 2eb64d10dc..facef17b63 100644 --- a/crates/collab/src/integration_tests.rs +++ b/crates/collab/src/integration_tests.rs @@ -2995,6 +2995,7 @@ async fn test_language_server_statuses( .unwrap(); let fake_language_server = fake_language_servers.next().await.unwrap(); + fake_language_server.start_progress("the-token").await; fake_language_server.notify::(lsp::ProgressParams { token: lsp::NumberOrString::String("the-token".to_string()), value: lsp::ProgressParamsValue::WorkDone(lsp::WorkDoneProgress::Report( diff --git a/crates/lsp/src/lsp.rs b/crates/lsp/src/lsp.rs index 90a6ff529a..0bef424104 100644 --- a/crates/lsp/src/lsp.rs +++ b/crates/lsp/src/lsp.rs @@ -655,6 +655,14 @@ impl FakeLanguageServer { self.server.notify::(params).ok(); } + pub async fn request(&self, params: T::Params) -> Result + where + T: request::Request, + T::Result: 'static + Send, + { + self.server.request::(params).await + } + pub async fn receive_notification(&mut self) -> T::Params { self.try_receive_notification::().await.unwrap() } @@ -708,14 +716,20 @@ impl FakeLanguageServer { self.server.remove_request_handler::(); } - pub async fn start_progress(&mut self, token: impl Into) { + pub async fn start_progress(&self, token: impl Into) { + let token = token.into(); + self.request::(WorkDoneProgressCreateParams { + token: NumberOrString::String(token.clone()), + }) + .await + .unwrap(); self.notify::(ProgressParams { - token: NumberOrString::String(token.into()), + token: NumberOrString::String(token), value: ProgressParamsValue::WorkDone(WorkDoneProgress::Begin(Default::default())), }); } - pub async fn end_progress(&mut self, token: impl Into) { + pub fn end_progress(&self, token: impl Into) { self.notify::(ProgressParams { token: NumberOrString::String(token.into()), value: ProgressParamsValue::WorkDone(WorkDoneProgress::End(Default::default())), diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index 9fa0c3fd45..dfb5e9bc80 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -178,7 +178,8 @@ pub enum Event { pub struct LanguageServerStatus { pub name: String, pub pending_work: BTreeMap, - pub pending_diagnostic_updates: isize, + pub has_pending_diagnostic_updates: bool, + progress_tokens: HashSet, } #[derive(Clone, Debug, Serialize)] @@ -546,7 +547,8 @@ impl Project { LanguageServerStatus { name: server.name, pending_work: Default::default(), - pending_diagnostic_updates: 0, + has_pending_diagnostic_updates: false, + progress_tokens: Default::default(), }, ) }) @@ -2025,8 +2027,23 @@ impl Project { // avoid stalling any language server like `gopls` which waits for a response // to these requests when initializing. language_server - .on_request::(|_, _| async { - Ok(()) + .on_request::({ + let this = this.downgrade(); + move |params, mut cx| async move { + if let Some(this) = this.upgrade(&cx) { + this.update(&mut cx, |this, _| { + if let Some(status) = + this.language_server_statuses.get_mut(&server_id) + { + if let lsp::NumberOrString::String(token) = params.token + { + status.progress_tokens.insert(token); + } + } + }); + } + Ok(()) + } }) .detach(); language_server @@ -2079,7 +2096,8 @@ impl Project { LanguageServerStatus { name: language_server.name().to_string(), pending_work: Default::default(), - pending_diagnostic_updates: 0, + has_pending_diagnostic_updates: false, + progress_tokens: Default::default(), }, ); language_server @@ -2291,19 +2309,22 @@ impl Project { } else { return; }; + + if !language_server_status.progress_tokens.contains(&token) { + return; + } + match progress { lsp::WorkDoneProgress::Begin(report) => { if Some(token.as_str()) == disk_based_diagnostics_progress_token { - language_server_status.pending_diagnostic_updates += 1; - if language_server_status.pending_diagnostic_updates == 1 { - self.disk_based_diagnostics_started(server_id, cx); - self.broadcast_language_server_update( - server_id, - proto::update_language_server::Variant::DiskBasedDiagnosticsUpdating( - proto::LspDiskBasedDiagnosticsUpdating {}, - ), - ); - } + language_server_status.has_pending_diagnostic_updates = true; + self.disk_based_diagnostics_started(server_id, cx); + self.broadcast_language_server_update( + server_id, + proto::update_language_server::Variant::DiskBasedDiagnosticsUpdating( + proto::LspDiskBasedDiagnosticsUpdating {}, + ), + ); } else { self.on_lsp_work_start( server_id, @@ -2350,17 +2371,17 @@ impl Project { } } lsp::WorkDoneProgress::End(_) => { + language_server_status.progress_tokens.remove(&token); + if Some(token.as_str()) == disk_based_diagnostics_progress_token { - language_server_status.pending_diagnostic_updates -= 1; - if language_server_status.pending_diagnostic_updates == 0 { - self.disk_based_diagnostics_finished(server_id, cx); - self.broadcast_language_server_update( - server_id, - proto::update_language_server::Variant::DiskBasedDiagnosticsUpdated( - proto::LspDiskBasedDiagnosticsUpdated {}, - ), - ); - } + language_server_status.has_pending_diagnostic_updates = false; + self.disk_based_diagnostics_finished(server_id, cx); + self.broadcast_language_server_update( + server_id, + proto::update_language_server::Variant::DiskBasedDiagnosticsUpdated( + proto::LspDiskBasedDiagnosticsUpdated {}, + ), + ); } else { self.on_lsp_work_end(server_id, token.clone(), cx); self.broadcast_language_server_update( @@ -4216,7 +4237,7 @@ impl Project { self.language_server_statuses .iter() .filter_map(|(id, status)| { - if status.pending_diagnostic_updates > 0 { + if status.has_pending_diagnostic_updates { Some(*id) } else { None @@ -4616,7 +4637,8 @@ impl Project { LanguageServerStatus { name: server.name, pending_work: Default::default(), - pending_diagnostic_updates: 0, + has_pending_diagnostic_updates: false, + progress_tokens: Default::default(), }, ); cx.notify(); @@ -6431,7 +6453,7 @@ mod tests { let mut events = subscribe(&project, cx); - let mut fake_server = fake_servers.next().await.unwrap(); + let fake_server = fake_servers.next().await.unwrap(); fake_server.start_progress(progress_token).await; assert_eq!( events.next().await.unwrap(), @@ -6440,10 +6462,6 @@ mod tests { } ); - fake_server.start_progress(progress_token).await; - fake_server.end_progress(progress_token).await; - fake_server.start_progress(progress_token).await; - fake_server.notify::( lsp::PublishDiagnosticsParams { uri: Url::from_file_path("/dir/a.rs").unwrap(), @@ -6464,8 +6482,7 @@ mod tests { } ); - fake_server.end_progress(progress_token).await; - fake_server.end_progress(progress_token).await; + fake_server.end_progress(progress_token); assert_eq!( events.next().await.unwrap(), Event::DiskBasedDiagnosticsFinished { @@ -6555,7 +6572,7 @@ mod tests { .unwrap(); // Simulate diagnostics starting to update. - let mut fake_server = fake_servers.next().await.unwrap(); + let fake_server = fake_servers.next().await.unwrap(); fake_server.start_progress(progress_token).await; // Restart the server before the diagnostics finish updating. @@ -6565,7 +6582,7 @@ mod tests { let mut events = subscribe(&project, cx); // Simulate the newly started server sending more diagnostics. - let mut fake_server = fake_servers.next().await.unwrap(); + let fake_server = fake_servers.next().await.unwrap(); fake_server.start_progress(progress_token).await; assert_eq!( events.next().await.unwrap(), @@ -6584,7 +6601,7 @@ mod tests { // All diagnostics are considered done, despite the old server's diagnostic // task never completing. - fake_server.end_progress(progress_token).await; + fake_server.end_progress(progress_token); assert_eq!( events.next().await.unwrap(), Event::DiskBasedDiagnosticsFinished {