refactor(lsp): remove RwLock on Config (#13485)

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<lsp::ConfigurationItem>,
+    specifiers: Vec<lsp::Url>,
-  ) -> Result<Vec<serde_json::Value>, AnyError> {
+  ) -> Result<Vec<Result<SpecifierSettings, AnyError>>, AnyError> {
-    self.0.configuration(items).await
+    self.0.specifier_configurations(specifiers).await
+  }
+
+  pub async fn specifier_configuration(
+    &self,
+    specifier: &lsp::Url,
+  ) -> Result<SpecifierSettings, AnyError> {
+    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<Value, AnyError> {
+    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<lsp::ConfigurationItem>,
+    uris: Vec<lsp::Url>,
-  ) -> AsyncReturn<Result<Vec<serde_json::Value>, AnyError>>;
+  ) -> AsyncReturn<Result<Vec<Result<SpecifierSettings, AnyError>>, AnyError>>;
+  fn workspace_configuration(&self) -> AsyncReturn<Result<Value, AnyError>>;
   fn show_message(
     &self,
     message_type: lsp::MessageType,
@@ -132,16 +165,56 @@ impl ClientTrait for LspowerClient {
     })
   }
 
-  fn configuration(
+  fn specifier_configurations(
     &self,
-    items: Vec<lsp::ConfigurationItem>,
+    uris: Vec<lsp::Url>,
-  ) -> AsyncReturn<Result<Vec<serde_json::Value>, AnyError>> {
+  ) -> AsyncReturn<Result<Vec<Result<SpecifierSettings, AnyError>>, AnyError>>
+  {
     let client = self.0.clone();
     Box::pin(async move {
-      client
+      let config_response = client
-        .configuration(items)
+        .configuration(
-        .await
+          uris
-        .map_err(|err| anyhow!("{}", err))
+            .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::<SpecifierSettings>(value).map_err(|err| {
+              anyhow!("Error converting specifier settings: {}", err)
+            })
+          })
+          .collect(),
+      )
+    })
+  }
+
+  fn workspace_configuration(&self) -> AsyncReturn<Result<Value, AnyError>> {
+    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<lsp::ConfigurationItem>,
+    uris: Vec<lsp::Url>,
-  ) -> AsyncReturn<Result<Vec<serde_json::Value>, AnyError>> {
+  ) -> AsyncReturn<Result<Vec<Result<SpecifierSettings, AnyError>>, AnyError>>
-    let is_global_config_request = items.len() == 1
+  {
-      && items[0].scope_uri.is_none()
+    // all specifiers are enabled for the REPL
-      && items[0].section.as_deref() == Some(SETTINGS_SECTION);
+    let settings = uris
-    let response = if is_global_config_request {
+      .into_iter()
-      vec![serde_json::to_value(get_repl_workspace_settings()).unwrap()]
+      .map(|_| {
-    } else {
+        Ok(SpecifierSettings {
-      // all specifiers are enabled for the REPL
+          enable: true,
-      items
+          ..Default::default()
-        .into_iter()
-        .map(|_| {
-          json!({
-            "enable": true,
-          })
         })
-        .collect()
+      })
-    };
+      .collect();
-    Box::pin(future::ready(Ok(response)))
+    Box::pin(future::ready(Ok(settings)))
+  }
+
+  fn workspace_configuration(&self) -> AsyncReturn<Result<Value, AnyError>> {
+    Box::pin(future::ready(Ok(
+      serde_json::to_value(get_repl_workspace_settings()).unwrap(),
+    )))
   }
 
   fn show_message(

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<RwLock<Settings>>,
+  settings: Settings,
-  tx: mpsc::Sender<ConfigRequest>,
   pub workspace_folders: Option<Vec<WorkspaceFolder>>,
 }
 
 impl Config {
-  pub fn new(client: Client) -> Self {
+  pub fn new() -> Self {
-    let (tx, mut rx) = mpsc::channel::<ConfigRequest>(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<lsp::ConfigurationItem>,
-              ) = {
-                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::<SpecifierSettings>(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::<SpecifierSettings>(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,
-                  );
-                }
-              }
-            }
-          }
-        }
-      })
-    });
-
     Self {
       client_capabilities: ClientCapabilities::default(),
-      settings,
+      settings: Default::default(),
-      tx,
       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<ConfigSnapshot> {
     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();
+    self
-    settings
+      .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 = self
-    let value = settings
+      .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 fn get_specifiers_with_client_uris(&self) -> Vec<SpecifierWithClientUri> {
-  pub async fn update_all_settings(&self) -> Result<(), AnyError> {
     self
-      .tx
+      .settings
-      .send(ConfigRequest::All)
+      .specifiers
-      .await
+      .iter()
-      .map_err(|_| anyhow!("Error sending config update task."))
+      .map(|(s, (u, _))| SpecifierWithClientUri {
+        specifier: s.clone(),
+        client_uri: u.clone(),
+      })
+      .collect::<Vec<_>>()
   }
 
-  /// Update a specific specifiers settings from the client.
+  pub fn set_specifier_settings(
-  pub async fn update_specifier_settings(
+    &mut self,
-    &self,
+    specifier: ModuleSpecifier,
-    specifier: &ModuleSpecifier,
+    client_uri: ModuleSpecifier,
-    uri: &ModuleSpecifier,
+    settings: SpecifierSettings,
-  ) -> Result<(), AnyError> {
+  ) {
     self
-      .tx
+      .settings
-      .send(ConfigRequest::Specifier(specifier.clone(), uri.clone()))
+      .specifiers
-      .await
+      .insert(specifier, (client_uri, settings));
-      .map_err(|_| anyhow!("Error sending config update task."))
   }
 }
 
@@ -466,17 +387,12 @@ mod tests {
   }
 
   fn setup() -> Config {
-    let mut maybe_client: Option<Client> = None;
+    Config::new()
-    let (_service, _) = lspower::LspService::new(|client| {
-      maybe_client = Some(Client::from_lspower(client));
-      MockLanguageServer::default()
-    });
-    Config::new(maybe_client.unwrap())
   }
 
   #[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");

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(&params));
-    let specifier = self.url_map.normalize_url(&params.text_document.uri);
-
-    if let Err(err) = self
-      .config
-      .update_specifier_settings(&specifier, &params.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<Value>,
     params: DidChangeConfigurationParams,
   ) {
-    let mark = self
-      .performance
-      .mark("did_change_configuration", Some(&params));
-
     let maybe_config =
       if self.config.client_capabilities.workspace_configuration {
-        let config_response = self
+        client_workspace_config
-          .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
-          }
-        }
       } 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(&params));
+
+      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(