From f066dd268fd9b99e373edf963f2f7b97631fb2cb Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Mon, 11 Mar 2024 16:30:30 +0100 Subject: [PATCH] Fix race when language server registers for `workspace/didChangeWatchedFiles` (#9177) This fixes #8896 by storing the `watched_paths` in a separate HashMap, allowing us to handle the request even before we mark the language server as running. Downside is that we have yet another data structure for language servers, but it also makes the `Running` enum case a bit smaller. And it fixes the race condition. Release Notes: - Fixed language servers not being notified of file changes if language server registers for file-notification right after starting up. ([#8896](https://github.com/zed-industries/zed/issues/8896)). Co-authored-by: Bennet Co-authored-by: Remco --- crates/project/src/project.rs | 135 ++++++++++++++++------------------ 1 file changed, 64 insertions(+), 71 deletions(-) diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index a21c864f127a25037fb385523134aa1d1c237c13..733590709a8ee896c50fe71b93c6a0ae3a3b8ad1 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -131,6 +131,7 @@ pub struct Project { language_server_ids: HashMap<(WorktreeId, LanguageServerName), LanguageServerId>, language_server_statuses: BTreeMap, last_workspace_edits_by_language_server: HashMap, + language_server_watched_paths: HashMap>, client: Arc, next_entry_id: Arc, join_project_response_message_id: u32, @@ -305,7 +306,6 @@ pub enum LanguageServerState { language: Arc, adapter: Arc, server: Arc, - watched_paths: HashMap, simulate_disk_based_diagnostics_completion: Option>, }, } @@ -601,6 +601,7 @@ impl Project { language_server_ids: HashMap::default(), language_server_statuses: Default::default(), last_workspace_edits_by_language_server: Default::default(), + language_server_watched_paths: HashMap::default(), buffers_being_formatted: Default::default(), buffers_needing_diff: Default::default(), git_diff_debouncer: DebouncedDelay::new(), @@ -732,6 +733,7 @@ impl Project { }) .collect(), last_workspace_edits_by_language_server: Default::default(), + language_server_watched_paths: HashMap::default(), opened_buffers: Default::default(), buffers_being_formatted: Default::default(), buffers_needing_diff: Default::default(), @@ -3317,7 +3319,6 @@ impl Project { LanguageServerState::Running { adapter: adapter.clone(), language: language.clone(), - watched_paths: Default::default(), server: language_server.clone(), simulate_disk_based_diagnostics_completion: None, }, @@ -3462,13 +3463,14 @@ impl Project { } } + self.language_server_watched_paths.remove(&server_id); self.language_server_statuses.remove(&server_id); cx.notify(); let server_state = self.language_servers.remove(&server_id); cx.emit(Event::LanguageServerRemoved(server_id)); - cx.spawn(move |this, cx| async move { - Self::shutdown_language_server(this, server_state, name, server_id, cx).await; + cx.spawn(move |_, cx| async move { + Self::shutdown_language_server(server_state, name, cx).await; orphaned_worktrees }) } else { @@ -3477,11 +3479,9 @@ impl Project { } async fn shutdown_language_server( - this: WeakModel, server_state: Option, name: Arc, - server_id: LanguageServerId, - mut cx: AsyncAppContext, + cx: AsyncAppContext, ) { let server = match server_state { Some(LanguageServerState::Starting(task)) => { @@ -3512,14 +3512,6 @@ impl Project { shutdown.await; } } - - if let Some(this) = this.upgrade() { - this.update(&mut cx, |this, cx| { - this.language_server_statuses.remove(&server_id); - cx.notify(); - }) - .ok(); - } } pub fn restart_language_servers_for_buffers( @@ -3890,64 +3882,63 @@ impl Project { params: DidChangeWatchedFilesRegistrationOptions, cx: &mut ModelContext, ) { - if let Some(LanguageServerState::Running { watched_paths, .. }) = - self.language_servers.get_mut(&language_server_id) - { - let mut builders = HashMap::default(); - for watcher in params.watchers { - for worktree in &self.worktrees { - if let Some(worktree) = worktree.upgrade() { - let glob_is_inside_worktree = worktree.update(cx, |tree, _| { - if let Some(abs_path) = tree.abs_path().to_str() { - let relative_glob_pattern = match &watcher.glob_pattern { - lsp::GlobPattern::String(s) => s - .strip_prefix(abs_path) - .and_then(|s| s.strip_prefix(std::path::MAIN_SEPARATOR)), - lsp::GlobPattern::Relative(rp) => { - let base_uri = match &rp.base_uri { - lsp::OneOf::Left(workspace_folder) => { - &workspace_folder.uri - } - lsp::OneOf::Right(base_uri) => base_uri, - }; - base_uri.to_file_path().ok().and_then(|file_path| { - (file_path.to_str() == Some(abs_path)) - .then_some(rp.pattern.as_str()) - }) - } - }; - if let Some(relative_glob_pattern) = relative_glob_pattern { - let literal_prefix = glob_literal_prefix(relative_glob_pattern); - tree.as_local_mut() - .unwrap() - .add_path_prefix_to_scan(Path::new(literal_prefix).into()); - if let Some(glob) = Glob::new(relative_glob_pattern).log_err() { - builders - .entry(tree.id()) - .or_insert_with(|| GlobSetBuilder::new()) - .add(glob); - } - return true; + let watched_paths = self + .language_server_watched_paths + .entry(language_server_id) + .or_default(); + + let mut builders = HashMap::default(); + for watcher in params.watchers { + for worktree in &self.worktrees { + if let Some(worktree) = worktree.upgrade() { + let glob_is_inside_worktree = worktree.update(cx, |tree, _| { + if let Some(abs_path) = tree.abs_path().to_str() { + let relative_glob_pattern = match &watcher.glob_pattern { + lsp::GlobPattern::String(s) => s + .strip_prefix(abs_path) + .and_then(|s| s.strip_prefix(std::path::MAIN_SEPARATOR)), + lsp::GlobPattern::Relative(rp) => { + let base_uri = match &rp.base_uri { + lsp::OneOf::Left(workspace_folder) => &workspace_folder.uri, + lsp::OneOf::Right(base_uri) => base_uri, + }; + base_uri.to_file_path().ok().and_then(|file_path| { + (file_path.to_str() == Some(abs_path)) + .then_some(rp.pattern.as_str()) + }) } + }; + if let Some(relative_glob_pattern) = relative_glob_pattern { + let literal_prefix = glob_literal_prefix(relative_glob_pattern); + tree.as_local_mut() + .unwrap() + .add_path_prefix_to_scan(Path::new(literal_prefix).into()); + if let Some(glob) = Glob::new(relative_glob_pattern).log_err() { + builders + .entry(tree.id()) + .or_insert_with(|| GlobSetBuilder::new()) + .add(glob); + } + return true; } - false - }); - if glob_is_inside_worktree { - break; } + false + }); + if glob_is_inside_worktree { + break; } } } + } - watched_paths.clear(); - for (worktree_id, builder) in builders { - if let Ok(globset) = builder.build() { - watched_paths.insert(worktree_id, globset); - } + watched_paths.clear(); + for (worktree_id, builder) in builders { + if let Ok(globset) = builder.build() { + watched_paths.insert(worktree_id, globset); } - - cx.notify(); } + + cx.notify(); } async fn on_lsp_workspace_edit( @@ -6731,6 +6722,8 @@ impl Project { self.language_server_ids .remove(&(id_to_remove, server_name)); self.language_server_statuses.remove(&server_id_to_remove); + self.language_server_watched_paths + .remove(&server_id_to_remove); self.last_workspace_edits_by_language_server .remove(&server_id_to_remove); self.language_servers.remove(&server_id_to_remove); @@ -6985,13 +6978,14 @@ impl Project { let abs_path = worktree_handle.read(cx).abs_path(); for server_id in &language_server_ids { - if let Some(LanguageServerState::Running { - server, - watched_paths, - .. - }) = self.language_servers.get(server_id) + if let Some(LanguageServerState::Running { server, .. }) = + self.language_servers.get(server_id) { - if let Some(watched_paths) = watched_paths.get(&worktree_id) { + if let Some(watched_paths) = self + .language_server_watched_paths + .get(&server_id) + .and_then(|paths| paths.get(&worktree_id)) + { let params = lsp::DidChangeWatchedFilesParams { changes: changes .iter() @@ -7013,7 +7007,6 @@ impl Project { }) .collect(), }; - if !params.changes.is_empty() { server .notify::(params)