From d4dd9ae4cfbcf83da952bf1ed99198c073f3e6d1 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 25 Jan 2022 10:30:38 -0500 Subject: [PATCH] refactor(lsp): remove RwLock on `Config` (#13485) --- cli/lsp/client.rs | 142 ++++++++++++++++++++++------- cli/lsp/config.rs | 178 ++++++++++--------------------------- cli/lsp/language_server.rs | 156 ++++++++++++++++++++++++-------- 3 files changed, 274 insertions(+), 202 deletions(-) diff --git a/cli/lsp/client.rs b/cli/lsp/client.rs index 1023048799..c5444c8ec6 100644 --- a/cli/lsp/client.rs +++ b/cli/lsp/client.rs @@ -3,15 +3,19 @@ use std::pin::Pin; use std::sync::Arc; use deno_core::anyhow::anyhow; +use deno_core::anyhow::bail; use deno_core::error::AnyError; use deno_core::futures::future; use deno_core::serde_json; use deno_core::serde_json::json; +use deno_core::serde_json::Value; use lspower::lsp; +use lspower::lsp::ConfigurationItem; -use crate::lsp::config::SETTINGS_SECTION; use crate::lsp::repl::get_repl_workspace_settings; +use super::config::SpecifierSettings; +use super::config::SETTINGS_SECTION; use super::lsp_custom; #[derive(Clone)] @@ -48,11 +52,39 @@ impl Client { self.0.send_registry_state_notification(params).await; } - pub async fn configuration( + pub async fn specifier_configurations( &self, - items: Vec, - ) -> Result, AnyError> { - self.0.configuration(items).await + specifiers: Vec, + ) -> Result>, AnyError> { + self.0.specifier_configurations(specifiers).await + } + + pub async fn specifier_configuration( + &self, + specifier: &lsp::Url, + ) -> Result { + let values = self + .0 + .specifier_configurations(vec![specifier.clone()]) + .await?; + if let Some(value) = values.into_iter().next() { + value.map_err(|err| { + anyhow!( + "Error converting specifier settings ({}): {}", + specifier, + err + ) + }) + } else { + bail!( + "Expected the client to return a configuration item for specifier: {}", + specifier + ); + } + } + + pub async fn workspace_configuration(&self) -> Result { + self.0.workspace_configuration().await } pub async fn show_message( @@ -87,10 +119,11 @@ trait ClientTrait: Send + Sync { &self, params: lsp_custom::RegistryStateNotificationParams, ) -> AsyncReturn<()>; - fn configuration( + fn specifier_configurations( &self, - items: Vec, - ) -> AsyncReturn, AnyError>>; + uris: Vec, + ) -> AsyncReturn>, AnyError>>; + fn workspace_configuration(&self) -> AsyncReturn>; fn show_message( &self, message_type: lsp::MessageType, @@ -132,16 +165,56 @@ impl ClientTrait for LspowerClient { }) } - fn configuration( + fn specifier_configurations( &self, - items: Vec, - ) -> AsyncReturn, AnyError>> { + uris: Vec, + ) -> AsyncReturn>, AnyError>> + { let client = self.0.clone(); Box::pin(async move { - client - .configuration(items) - .await - .map_err(|err| anyhow!("{}", err)) + let config_response = client + .configuration( + uris + .into_iter() + .map(|uri| ConfigurationItem { + scope_uri: Some(uri), + section: Some(SETTINGS_SECTION.to_string()), + }) + .collect(), + ) + .await?; + + Ok( + config_response + .into_iter() + .map(|value| { + serde_json::from_value::(value).map_err(|err| { + anyhow!("Error converting specifier settings: {}", err) + }) + }) + .collect(), + ) + }) + } + + fn workspace_configuration(&self) -> AsyncReturn> { + let client = self.0.clone(); + Box::pin(async move { + let config_response = client + .configuration(vec![ConfigurationItem { + scope_uri: None, + section: Some(SETTINGS_SECTION.to_string()), + }]) + .await; + match config_response { + Ok(value_vec) => match value_vec.get(0).cloned() { + Some(value) => Ok(value), + None => bail!("Missing response workspace configuration."), + }, + Err(err) => { + bail!("Error getting workspace configuration: {}", err) + } + } }) } @@ -188,27 +261,28 @@ impl ClientTrait for ReplClient { Box::pin(future::ready(())) } - fn configuration( + fn specifier_configurations( &self, - items: Vec, - ) -> AsyncReturn, AnyError>> { - let is_global_config_request = items.len() == 1 - && items[0].scope_uri.is_none() - && items[0].section.as_deref() == Some(SETTINGS_SECTION); - let response = if is_global_config_request { - vec![serde_json::to_value(get_repl_workspace_settings()).unwrap()] - } else { - // all specifiers are enabled for the REPL - items - .into_iter() - .map(|_| { - json!({ - "enable": true, - }) + uris: Vec, + ) -> AsyncReturn>, AnyError>> + { + // all specifiers are enabled for the REPL + let settings = uris + .into_iter() + .map(|_| { + Ok(SpecifierSettings { + enable: true, + ..Default::default() }) - .collect() - }; - Box::pin(future::ready(Ok(response))) + }) + .collect(); + Box::pin(future::ready(Ok(settings))) + } + + fn workspace_configuration(&self) -> AsyncReturn> { + Box::pin(future::ready(Ok( + serde_json::to_value(get_repl_workspace_settings()).unwrap(), + ))) } fn show_message( diff --git a/cli/lsp/config.rs b/cli/lsp/config.rs index 37558e1b7a..1fef18bb34 100644 --- a/cli/lsp/config.rs +++ b/cli/lsp/config.rs @@ -226,6 +226,12 @@ enum ConfigRequest { Specifier(ModuleSpecifier, ModuleSpecifier), } +#[derive(Debug, Clone)] +pub struct SpecifierWithClientUri { + pub specifier: ModuleSpecifier, + pub client_uri: ModuleSpecifier, +} + #[derive(Debug, Default, Clone)] pub struct Settings { pub specifiers: @@ -236,149 +242,62 @@ pub struct Settings { #[derive(Debug)] pub struct Config { pub client_capabilities: ClientCapabilities, - settings: Arc>, - tx: mpsc::Sender, + settings: Settings, pub workspace_folders: Option>, } impl Config { - pub fn new(client: Client) -> Self { - let (tx, mut rx) = mpsc::channel::(100); - let settings = Arc::new(RwLock::new(Settings::default())); - let settings_ref = settings.clone(); - - let _join_handle = thread::spawn(move || { - let runtime = create_basic_runtime(); - - runtime.block_on(async { - loop { - match rx.recv().await { - None => break, - Some(ConfigRequest::All) => { - let (specifier_uri_map, items): ( - Vec<(ModuleSpecifier, ModuleSpecifier)>, - Vec, - ) = { - let settings = settings_ref.read(); - ( - settings - .specifiers - .iter() - .map(|(s, (u, _))| (s.clone(), u.clone())) - .collect(), - settings - .specifiers - .iter() - .map(|(_, (uri, _))| lsp::ConfigurationItem { - scope_uri: Some(uri.clone()), - section: Some(SETTINGS_SECTION.to_string()), - }) - .collect(), - ) - }; - if let Ok(configs) = client.configuration(items).await { - let mut settings = settings_ref.write(); - for (i, value) in configs.into_iter().enumerate() { - match serde_json::from_value::(value) { - Ok(specifier_settings) => { - let (specifier, uri) = specifier_uri_map[i].clone(); - settings - .specifiers - .insert(specifier, (uri, specifier_settings)); - } - Err(err) => { - error!("Error converting specifier settings: {}", err); - } - } - } - } - } - Some(ConfigRequest::Specifier(specifier, uri)) => { - if settings_ref.read().specifiers.contains_key(&specifier) { - continue; - } - match client - .configuration(vec![lsp::ConfigurationItem { - scope_uri: Some(uri.clone()), - section: Some(SETTINGS_SECTION.to_string()), - }]) - .await - { - Ok(values) => { - if let Some(value) = values.first() { - match serde_json::from_value::(value.clone()) { - Ok(specifier_settings) => { - settings_ref - .write() - .specifiers - .insert(specifier, (uri, specifier_settings)); - } - Err(err) => { - error!("Error converting specifier settings ({}): {}", specifier, err); - } - } - } else { - error!("Expected the client to return a configuration item for specifier: {}", specifier); - } - }, - Err(err) => { - error!( - "Error retrieving settings for specifier ({}): {}", - specifier, - err, - ); - } - } - } - } - } - }) - }); - + pub fn new() -> Self { Self { client_capabilities: ClientCapabilities::default(), - settings, - tx, + settings: Default::default(), workspace_folders: None, } } pub fn get_workspace_settings(&self) -> WorkspaceSettings { - self.settings.read().workspace.clone() + self.settings.workspace.clone() } /// Set the workspace settings directly, which occurs during initialization /// and when the client does not support workspace configuration requests - pub fn set_workspace_settings(&self, value: Value) -> Result<(), AnyError> { + pub fn set_workspace_settings( + &mut self, + value: Value, + ) -> Result<(), AnyError> { let workspace_settings = serde_json::from_value(value)?; - self.settings.write().workspace = workspace_settings; + self.settings.workspace = workspace_settings; Ok(()) } pub fn snapshot(&self) -> Arc { Arc::new(ConfigSnapshot { client_capabilities: self.client_capabilities.clone(), - settings: self.settings.read().clone(), + settings: self.settings.clone(), workspace_folders: self.workspace_folders.clone(), }) } + pub fn has_specifier_settings(&self, specifier: &ModuleSpecifier) -> bool { + self.settings.specifiers.contains_key(specifier) + } + pub fn specifier_enabled(&self, specifier: &ModuleSpecifier) -> bool { - let settings = self.settings.read(); - settings + self + .settings .specifiers .get(specifier) .map(|(_, s)| s.enable) - .unwrap_or_else(|| settings.workspace.enable) + .unwrap_or_else(|| self.settings.workspace.enable) } pub fn specifier_code_lens_test(&self, specifier: &ModuleSpecifier) -> bool { - let settings = self.settings.read(); - let value = settings + let value = self + .settings .specifiers .get(specifier) .map(|(_, s)| s.code_lens.test) - .unwrap_or_else(|| settings.workspace.code_lens.test); + .unwrap_or_else(|| self.settings.workspace.code_lens.test); value } @@ -416,26 +335,28 @@ impl Config { } } - /// Update all currently cached specifier settings - pub async fn update_all_settings(&self) -> Result<(), AnyError> { + pub fn get_specifiers_with_client_uris(&self) -> Vec { self - .tx - .send(ConfigRequest::All) - .await - .map_err(|_| anyhow!("Error sending config update task.")) + .settings + .specifiers + .iter() + .map(|(s, (u, _))| SpecifierWithClientUri { + specifier: s.clone(), + client_uri: u.clone(), + }) + .collect::>() } - /// Update a specific specifiers settings from the client. - pub async fn update_specifier_settings( - &self, - specifier: &ModuleSpecifier, - uri: &ModuleSpecifier, - ) -> Result<(), AnyError> { + pub fn set_specifier_settings( + &mut self, + specifier: ModuleSpecifier, + client_uri: ModuleSpecifier, + settings: SpecifierSettings, + ) { self - .tx - .send(ConfigRequest::Specifier(specifier.clone(), uri.clone())) - .await - .map_err(|_| anyhow!("Error sending config update task.")) + .settings + .specifiers + .insert(specifier, (client_uri, settings)); } } @@ -466,17 +387,12 @@ mod tests { } fn setup() -> Config { - let mut maybe_client: Option = None; - let (_service, _) = lspower::LspService::new(|client| { - maybe_client = Some(Client::from_lspower(client)); - MockLanguageServer::default() - }); - Config::new(maybe_client.unwrap()) + Config::new() } #[test] fn test_config_specifier_enabled() { - let config = setup(); + let mut config = setup(); let specifier = resolve_url("file:///a.ts").unwrap(); assert!(!config.specifier_enabled(&specifier)); config @@ -489,7 +405,7 @@ mod tests { #[test] fn test_set_workspace_settings_defaults() { - let config = setup(); + let mut config = setup(); config .set_workspace_settings(json!({})) .expect("could not update"); diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 5ebf20fda0..b21081e85e 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -148,7 +148,7 @@ impl Inner { let documents = Documents::new(&location); let performance = Arc::new(Performance::default()); let ts_server = Arc::new(TsServer::new(performance.clone())); - let config = Config::new(client.clone()); + let config = Config::new(); let diagnostics_server = DiagnosticsServer::new( client.clone(), performance.clone(), @@ -741,17 +741,12 @@ impl Inner { Ok(()) } - async fn did_open(&mut self, params: DidOpenTextDocumentParams) { + async fn did_open( + &mut self, + specifier: &ModuleSpecifier, + params: DidOpenTextDocumentParams, + ) { let mark = self.performance.mark("did_open", Some(¶ms)); - let specifier = self.url_map.normalize_url(¶ms.text_document.uri); - - if let Err(err) = self - .config - .update_specifier_settings(&specifier, ¶ms.text_document.uri) - .await - { - error!("Error updating specifier settings: {}", err); - } if params.text_document.uri.scheme() == "deno" { // we can ignore virtual text documents opening, as they don't need to @@ -785,7 +780,7 @@ impl Inner { if document.is_diagnosable() { self .diagnostics_server - .invalidate(self.documents.dependents(&specifier)) + .invalidate(self.documents.dependents(specifier)) .await; if let Err(err) = self.diagnostics_server.update() { error!("{}", err); @@ -845,31 +840,12 @@ impl Inner { async fn did_change_configuration( &mut self, + client_workspace_config: Option, params: DidChangeConfigurationParams, ) { - let mark = self - .performance - .mark("did_change_configuration", Some(¶ms)); - let maybe_config = if self.config.client_capabilities.workspace_configuration { - let config_response = self - .client - .configuration(vec![ConfigurationItem { - scope_uri: None, - section: Some(SETTINGS_SECTION.to_string()), - }]) - .await; - if let Err(err) = self.config.update_all_settings().await { - error!("Cannot request updating all settings: {}", err); - } - match config_response { - Ok(value_vec) => value_vec.get(0).cloned(), - Err(err) => { - error!("Error getting workspace configuration: {}", err); - None - } - } + client_workspace_config } else { params .settings @@ -908,8 +884,6 @@ impl Inner { self.maybe_import_map.clone(), self.maybe_config_file.as_ref(), ); - - self.performance.measure(mark); } async fn did_change_watched_files( @@ -2376,7 +2350,39 @@ impl lspower::LanguageServer for LanguageServer { } async fn did_open(&self, params: DidOpenTextDocumentParams) { - self.0.lock().await.did_open(params).await + let (client, uri, specifier, had_specifier_settings) = { + let mut inner = self.0.lock().await; + let client = inner.client.clone(); + let uri = params.text_document.uri.clone(); + let specifier = inner.url_map.normalize_url(&uri); + inner.did_open(&specifier, params).await; + let has_specifier_settings = + inner.config.has_specifier_settings(&specifier); + (client, uri, specifier, has_specifier_settings) + }; + + // retrieve the specifier settings outside the lock if + // they haven't been asked for yet on its own time + if !had_specifier_settings { + let language_server = self.clone(); + tokio::spawn(async move { + let response = client.specifier_configuration(&uri).await; + match response { + Ok(specifier_settings) => { + // now update the config + let mut inner = language_server.0.lock().await; + inner.config.set_specifier_settings( + specifier, + uri, + specifier_settings, + ); + } + Err(err) => { + error!("{}", err); + } + } + }); + } } async fn did_change(&self, params: DidChangeTextDocumentParams) { @@ -2396,7 +2402,83 @@ impl lspower::LanguageServer for LanguageServer { &self, params: DidChangeConfigurationParams, ) { - self.0.lock().await.did_change_configuration(params).await + let (has_workspace_capability, client, specifiers, mark) = { + let inner = self.0.lock().await; + let mark = inner + .performance + .mark("did_change_configuration", Some(¶ms)); + + let specifiers = + if inner.config.client_capabilities.workspace_configuration { + Some(inner.config.get_specifiers_with_client_uris()) + } else { + None + }; + ( + inner.config.client_capabilities.workspace_configuration, + inner.client.clone(), + specifiers, + mark, + ) + }; + + // start retreiving all the specifiers' settings outside the lock on its own time + if let Some(specifiers) = specifiers { + let language_server = self.clone(); + let client = client.clone(); + tokio::spawn(async move { + if let Ok(configs) = client + .specifier_configurations( + specifiers.iter().map(|(s)| s.client_uri.clone()).collect(), + ) + .await + { + let mut inner = language_server.0.lock().await; + for (i, value) in configs.into_iter().enumerate() { + match value { + Ok(specifier_settings) => { + let entry = specifiers[i].clone(); + inner.config.set_specifier_settings( + entry.specifier, + entry.client_uri, + specifier_settings, + ); + } + Err(err) => { + error!("{}", err); + } + } + } + } + }); + } + + // Get the configuration from the client outside of the lock + // in order to prevent potential deadlocking scenarios where + // the server holds a lock and calls into the client, which + // calls into the server which deadlocks acquiring the lock. + // There is a gap here between when the configuration is + // received and acquiring the lock, but most likely there + // won't be any racing here. + let client_workspace_config = if has_workspace_capability { + let config_response = client.workspace_configuration().await; + match config_response { + Ok(value) => Some(value), + Err(err) => { + error!("{}", err); + None + } + } + } else { + None + }; + + // now update the inner state + let mut inner = self.0.lock().await; + inner + .did_change_configuration(client_workspace_config, params) + .await; + inner.performance.measure(mark); } async fn did_change_watched_files(