refactor(lsp): cleanup partially locking methods (#23723)

cli/lsp/client.rs

@@ -50,6 +50,18 @@ impl Client {
     OutsideLockClient(self.0.clone())
   }
 
+  pub async fn publish_diagnostics(
+    &self,
+    uri: LspClientUrl,
+    diags: Vec<lsp::Diagnostic>,
+    version: Option<i32>,
+  ) {
+    self
+      .0
+      .publish_diagnostics(uri.into_url(), diags, version)
+      .await;
+  }
+
   pub fn send_registry_state_notification(
     &self,
     params: lsp_custom::RegistryStateNotificationParams,
@@ -141,18 +153,6 @@ impl OutsideLockClient {
   ) -> Result<Vec<WorkspaceSettings>, AnyError> {
     self.0.workspace_configuration(scopes).await
   }
-
-  pub async fn publish_diagnostics(
-    &self,
-    uri: LspClientUrl,
-    diags: Vec<lsp::Diagnostic>,
-    version: Option<i32>,
-  ) {
-    self
-      .0
-      .publish_diagnostics(uri.into_url(), diags, version)
-      .await;
-  }
 }
 
 #[async_trait]

cli/lsp/diagnostics.rs

@@ -155,7 +155,6 @@ impl DiagnosticsPublisher {
         .update(&record.specifier, version, &all_specifier_diagnostics);
       self
         .client
-        .when_outside_lsp_lock()
         .publish_diagnostics(
           url_map
             .normalize_specifier(&record.specifier)
@@ -186,7 +185,6 @@ impl DiagnosticsPublisher {
           self.state.update(specifier, removed_value.version, &[]);
           self
             .client
-            .when_outside_lsp_lock()
             .publish_diagnostics(
               url_map
                 .normalize_specifier(specifier)

cli/lsp/language_server.rs

@@ -747,12 +747,6 @@ impl Inner {
     }
 
     {
-      if let Some(options) = params.initialization_options {
-        self.config.set_workspace_settings(
-          WorkspaceSettings::from_initialization_options(options),
-          vec![],
-        );
-      }
       let mut workspace_folders = vec![];
       if let Some(folders) = params.workspace_folders {
         workspace_folders = folders
@@ -784,9 +778,16 @@ impl Inner {
         }
       }
       self.config.set_workspace_folders(workspace_folders);
+      if let Some(options) = params.initialization_options {
+        self.config.set_workspace_settings(
+          WorkspaceSettings::from_initialization_options(options),
+          vec![],
+        );
+      }
       self.config.update_capabilities(&params.capabilities);
     }
 
+    self.diagnostics_server.start();
     if let Err(e) = self
       .ts_server
       .start(self.config.internal_inspect().to_address())
@@ -1023,12 +1024,14 @@ impl Inner {
     Ok(())
   }
 
-  fn did_open(
+  async fn did_open(&mut self, params: DidOpenTextDocumentParams) {
-    &mut self,
-    specifier: &ModuleSpecifier,
-    params: DidOpenTextDocumentParams,
-  ) -> Arc<Document> {
     let mark = self.performance.mark_with_args("lsp.did_open", &params);
+    if params.text_document.uri.scheme() == "deno" {
+      // we can ignore virtual text documents opening, as they don't need to
+      // be tracked in memory, as they are static assets that won't change
+      // already managed by the language service
+      return;
+    }
     let language_id =
       params
         .text_document
@@ -1045,6 +1048,9 @@ impl Inner {
         params.text_document.uri
       );
     }
+    let specifier = self
+      .url_map
+      .normalize_url(&params.text_document.uri, LspUrlKind::File);
     let document = self.documents.open(
       specifier.clone(),
       params.text_document.version,
@@ -1052,9 +1058,13 @@ impl Inner {
       params.text_document.text.into(),
     );
     self.project_changed([(document.specifier(), ChangeKind::Opened)], false);
-
+    if document.is_diagnosable() {
+      self.refresh_npm_specifiers().await;
+      self.diagnostics_server.invalidate(&[specifier]);
+      self.send_diagnostics_update();
+      self.send_testing_update();
+    }
     self.performance.measure(mark);
-    document
   }
 
   async fn did_change(&mut self, params: DidChangeTextDocumentParams) {
@@ -1084,6 +1094,34 @@ impl Inner {
     self.performance.measure(mark);
   }
 
+  fn did_save(&mut self, params: DidSaveTextDocumentParams) {
+    let _mark = self.performance.measure_scope("lsp.did_save");
+    let specifier = self
+      .url_map
+      .normalize_url(&params.text_document.uri, LspUrlKind::File);
+    self.documents.save(&specifier);
+    if !self
+      .config
+      .workspace_settings_for_specifier(&specifier)
+      .cache_on_save
+      || !self.config.specifier_enabled(&specifier)
+      || !self.diagnostics_state.has_no_cache_diagnostics(&specifier)
+    {
+      return;
+    }
+    match specifier_to_file_path(&specifier) {
+      Ok(path) if is_importable_ext(&path) => {}
+      _ => return,
+    }
+    self.task_queue.queue_task(Box::new(|ls: LanguageServer| {
+      spawn(async move {
+        if let Err(err) = ls.cache(vec![], specifier.clone(), false).await {
+          lsp_warn!("Failed to cache \"{}\" on save: {:#}", &specifier, err);
+        }
+      });
+    }));
+  }
+
   async fn refresh_npm_specifiers(&mut self) {
     let package_reqs = self.documents.npm_package_reqs();
     let resolver = self.resolver.clone();
@@ -1118,37 +1156,6 @@ impl Inner {
     self.performance.measure(mark);
   }
 
-  async fn did_change_configuration(
-    &mut self,
-    params: DidChangeConfigurationParams,
-  ) {
-    if !self.config.client_capabilities.workspace_configuration {
-      let config = params.settings.as_object().map(|settings| {
-        let deno =
-          serde_json::to_value(settings.get(SETTINGS_SECTION)).unwrap();
-        let javascript =
-          serde_json::to_value(settings.get("javascript")).unwrap();
-        let typescript =
-          serde_json::to_value(settings.get("typescript")).unwrap();
-        WorkspaceSettings::from_raw_settings(deno, javascript, typescript)
-      });
-      if let Some(settings) = config {
-        self.config.set_workspace_settings(settings, vec![]);
-      }
-    };
-
-    self.update_debug_flag();
-    self.update_global_cache().await;
-    self.refresh_workspace_files();
-    self.refresh_config_tree().await;
-    self.update_cache();
-    self.refresh_resolver().await;
-    self.refresh_documents_config().await;
-    self.diagnostics_server.invalidate_all();
-    self.send_diagnostics_update();
-    self.send_testing_update();
-  }
-
   async fn did_change_watched_files(
     &mut self,
     params: DidChangeWatchedFilesParams,
@@ -1233,32 +1240,6 @@ impl Inner {
     self.performance.measure(mark);
   }
 
-  fn did_change_workspace_folders(
-    &mut self,
-    params: DidChangeWorkspaceFoldersParams,
-  ) {
-    let mut workspace_folders = params
-      .event
-      .added
-      .into_iter()
-      .map(|folder| {
-        (
-          self.url_map.normalize_url(&folder.uri, LspUrlKind::Folder),
-          folder,
-        )
-      })
-      .collect::<Vec<(ModuleSpecifier, WorkspaceFolder)>>();
-    for (specifier, folder) in &self.config.workspace_folders {
-      if !params.event.removed.is_empty()
-        && params.event.removed.iter().any(|f| f.uri == folder.uri)
-      {
-        continue;
-      }
-      workspace_folders.push((specifier.clone(), folder.clone()));
-    }
-    self.config.set_workspace_folders(workspace_folders);
-  }
-
   async fn document_symbol(
     &self,
     params: DocumentSymbolParams,
@@ -2866,9 +2847,7 @@ impl tower_lsp::LanguageServer for LanguageServer {
     &self,
     params: InitializeParams,
   ) -> LspResult<InitializeResult> {
-    let mut language_server = self.0.write().await;
+    self.0.write().await.initialize(params).await
-    language_server.diagnostics_server.start();
-    language_server.initialize(params).await
   }
 
   async fn initialized(&self, _: InitializedParams) {
@@ -3007,24 +2986,7 @@ impl tower_lsp::LanguageServer for LanguageServer {
   }
 
   async fn did_open(&self, params: DidOpenTextDocumentParams) {
-    if params.text_document.uri.scheme() == "deno" {
+    self.0.write().await.did_open(params).await;
-      // we can ignore virtual text documents opening, as they don't need to
-      // be tracked in memory, as they are static assets that won't change
-      // already managed by the language service
-      return;
-    }
-
-    let mut inner = self.0.write().await;
-    let specifier = inner
-      .url_map
-      .normalize_url(&params.text_document.uri, LspUrlKind::File);
-    let document = inner.did_open(&specifier, params);
-    if document.is_diagnosable() {
-      inner.refresh_npm_specifiers().await;
-      inner.diagnostics_server.invalidate(&[specifier]);
-      inner.send_diagnostics_update();
-      inner.send_testing_update();
-    }
   }
 
   async fn did_change(&self, params: DidChangeTextDocumentParams) {
@@ -3032,29 +2994,7 @@ impl tower_lsp::LanguageServer for LanguageServer {
   }
 
   async fn did_save(&self, params: DidSaveTextDocumentParams) {
-    let uri = &params.text_document.uri;
+    self.0.write().await.did_save(params);
-    let specifier = {
-      let mut inner = self.0.write().await;
-      let specifier = inner.url_map.normalize_url(uri, LspUrlKind::File);
-      inner.documents.save(&specifier);
-      if !inner
-        .config
-        .workspace_settings_for_specifier(&specifier)
-        .cache_on_save
-        || !inner.config.specifier_enabled(&specifier)
-        || !inner.diagnostics_state.has_no_cache_diagnostics(&specifier)
-      {
-        return;
-      }
-      match specifier_to_file_path(&specifier) {
-        Ok(path) if is_importable_ext(&path) => {}
-        _ => return,
-      }
-      specifier
-    };
-    if let Err(err) = self.cache(vec![], specifier.clone(), false).await {
-      lsp_warn!("Failed to cache \"{}\" on save: {:#}", &specifier, err);
-    }
   }
 
   async fn did_close(&self, params: DidCloseTextDocumentParams) {
@@ -3071,11 +3011,32 @@ impl tower_lsp::LanguageServer for LanguageServer {
         .performance
         .mark_with_args("lsp.did_change_configuration", &params)
     };
-
     self.refresh_configuration().await;
-
     let mut inner = self.0.write().await;
-    inner.did_change_configuration(params).await;
+    if !inner.config.client_capabilities.workspace_configuration {
+      let config = params.settings.as_object().map(|settings| {
+        let deno =
+          serde_json::to_value(settings.get(SETTINGS_SECTION)).unwrap();
+        let javascript =
+          serde_json::to_value(settings.get("javascript")).unwrap();
+        let typescript =
+          serde_json::to_value(settings.get("typescript")).unwrap();
+        WorkspaceSettings::from_raw_settings(deno, javascript, typescript)
+      });
+      if let Some(settings) = config {
+        inner.config.set_workspace_settings(settings, vec![]);
+      }
+    };
+    inner.update_debug_flag();
+    inner.update_global_cache().await;
+    inner.refresh_workspace_files();
+    inner.refresh_config_tree().await;
+    inner.update_cache();
+    inner.refresh_resolver().await;
+    inner.refresh_documents_config().await;
+    inner.diagnostics_server.invalidate_all();
+    inner.send_diagnostics_update();
+    inner.send_testing_update();
     inner.performance.measure(mark);
   }
 
@@ -3090,26 +3051,43 @@ impl tower_lsp::LanguageServer for LanguageServer {
     &self,
     params: DidChangeWorkspaceFoldersParams,
   ) {
-    let (performance, mark) = {
+    let mark = {
-      let mut ls = self.0.write().await;
+      let mut inner = self.0.write().await;
-      let mark = ls
+      let mark = inner
         .performance
         .mark_with_args("lsp.did_change_workspace_folders", &params);
-      ls.did_change_workspace_folders(params);
+      let mut workspace_folders = params
-      (ls.performance.clone(), mark)
+        .event
+        .added
+        .into_iter()
+        .map(|folder| {
+          (
+            inner.url_map.normalize_url(&folder.uri, LspUrlKind::Folder),
+            folder,
+          )
+        })
+        .collect::<Vec<(ModuleSpecifier, WorkspaceFolder)>>();
+      for (specifier, folder) in &inner.config.workspace_folders {
+        if !params.event.removed.is_empty()
+          && params.event.removed.iter().any(|f| f.uri == folder.uri)
+        {
+          continue;
+        }
+        workspace_folders.push((specifier.clone(), folder.clone()));
+      }
+      inner.config.set_workspace_folders(workspace_folders);
+      mark
     };
-
     self.refresh_configuration().await;
-    {
+    let mut inner = self.0.write().await;
-      let mut ls = self.0.write().await;
+    inner.refresh_workspace_files();
-      ls.refresh_workspace_files();
+    inner.refresh_config_tree().await;
-      ls.refresh_config_tree().await;
+    inner.refresh_resolver().await;
-      ls.refresh_resolver().await;
+    inner.refresh_documents_config().await;
-      ls.refresh_documents_config().await;
+    inner.diagnostics_server.invalidate_all();
-      ls.diagnostics_server.invalidate_all();
+    inner.send_diagnostics_update();
-      ls.send_diagnostics_update();
+    inner.send_testing_update();
-    }
+    inner.performance.measure(mark);
-    performance.measure(mark);
   }
 
   async fn document_symbol(

cli/lsp/performance.rs

@@ -8,6 +8,7 @@ use std::cmp;
 use std::collections::HashMap;
 use std::collections::VecDeque;
 use std::fmt;
+use std::sync::Arc;
 use std::time::Duration;
 use std::time::Instant;
 
@@ -70,22 +71,56 @@ impl From<PerformanceMark> for PerformanceMeasure {
   }
 }
 
-/// A simple structure for marking a start of something to measure the duration
-/// of and measuring that duration.  Each measurement is identified by a string
-/// name and a counter is incremented each time a new measurement is marked.
-///
-/// The structure will limit the size of measurements to the most recent 1000,
-/// and will roll off when that limit is reached.
 #[derive(Debug)]
-pub struct Performance {
+pub struct PerformanceScopeMark {
-  counts: Mutex<HashMap<String, u32>>,
+  performance_inner: Arc<Mutex<PerformanceInner>>,
-  measurements_by_type:
+  inner: Option<PerformanceMark>,
-    Mutex<HashMap<String, (/* count */ u32, /* duration */ f64)>>,
-  max_size: usize,
-  measures: Mutex<VecDeque<PerformanceMeasure>>,
 }
 
-impl Default for Performance {
+impl Drop for PerformanceScopeMark {
+  fn drop(&mut self) {
+    self
+      .performance_inner
+      .lock()
+      .measure(self.inner.take().unwrap());
+  }
+}
+
+#[derive(Debug)]
+struct PerformanceInner {
+  counts: HashMap<String, u32>,
+  measurements_by_type: HashMap<String, (/* count */ u32, /* duration */ f64)>,
+  max_size: usize,
+  measures: VecDeque<PerformanceMeasure>,
+}
+
+impl PerformanceInner {
+  fn measure(&mut self, mark: PerformanceMark) -> Duration {
+    let measure = PerformanceMeasure::from(mark);
+    lsp_debug!(
+      "{},",
+      json!({
+        "type": "measure",
+        "name": measure.name,
+        "count": measure.count,
+        "duration": measure.duration.as_micros() as f64 / 1000.0,
+      })
+    );
+    let duration = measure.duration;
+    let measurement = self
+      .measurements_by_type
+      .entry(measure.name.to_string())
+      .or_insert((0, 0.0));
+    measurement.1 += duration.as_micros() as f64 / 1000.0;
+    self.measures.push_front(measure);
+    while self.measures.len() > self.max_size {
+      self.measures.pop_back();
+    }
+    duration
+  }
+}
+
+impl Default for PerformanceInner {
   fn default() -> Self {
     Self {
       counts: Default::default(),
@@ -96,12 +131,21 @@ impl Default for Performance {
   }
 }
 
+/// A simple structure for marking a start of something to measure the duration
+/// of and measuring that duration.  Each measurement is identified by a string
+/// name and a counter is incremented each time a new measurement is marked.
+///
+/// The structure will limit the size of measurements to the most recent 1000,
+/// and will roll off when that limit is reached.
+#[derive(Debug, Default)]
+pub struct Performance(Arc<Mutex<PerformanceInner>>);
+
 impl Performance {
   /// Return the count and average duration of a measurement identified by name.
   #[cfg(test)]
   pub fn average(&self, name: &str) -> Option<(usize, Duration)> {
     let mut items = Vec::new();
-    for measure in self.measures.lock().iter() {
+    for measure in self.0.lock().measures.iter() {
       if measure.name == name {
         items.push(measure.duration);
       }
@@ -120,7 +164,7 @@ impl Performance {
   /// of each measurement.
   pub fn averages(&self) -> Vec<PerformanceAverage> {
     let mut averages: HashMap<String, Vec<Duration>> = HashMap::new();
-    for measure in self.measures.lock().iter() {
+    for measure in self.0.lock().measures.iter() {
       averages
         .entry(measure.name.clone())
         .or_default()
@@ -141,8 +185,10 @@ impl Performance {
   }
 
   pub fn measurements_by_type(&self) -> Vec<(String, u32, f64)> {
-    let measurements_by_type = self.measurements_by_type.lock();
+    self
-    measurements_by_type
+      .0
+      .lock()
+      .measurements_by_type
       .iter()
       .map(|(name, (count, duration))| (name.to_string(), *count, *duration))
       .collect::<Vec<_>>()
@@ -150,7 +196,7 @@ impl Performance {
 
   pub fn averages_as_f64(&self) -> Vec<(String, u32, f64)> {
     let mut averages: HashMap<String, Vec<Duration>> = HashMap::new();
-    for measure in self.measures.lock().iter() {
+    for measure in self.0.lock().measures.iter() {
       averages
         .entry(measure.name.clone())
         .or_default()
@@ -171,17 +217,18 @@ impl Performance {
     name: S,
     maybe_args: Option<V>,
   ) -> PerformanceMark {
+    let mut inner = self.0.lock();
     let name = name.as_ref();
-    let mut counts = self.counts.lock();
+    let count = *inner
-    let count = counts.entry(name.to_string()).or_insert(0);
+      .counts
-    *count += 1;
+      .entry(name.to_string())
-    {
+      .and_modify(|c| *c += 1)
-      let mut measurements_by_type = self.measurements_by_type.lock();
+      .or_insert(1);
-      let measurement = measurements_by_type
+    inner
-        .entry(name.to_string())
+      .measurements_by_type
-        .or_insert((0, 0.0));
+      .entry(name.to_string())
-      measurement.0 += 1;
+      .and_modify(|(c, _)| *c += 1)
-    }
+      .or_insert((1, 0.0));
     let msg = if let Some(args) = maybe_args {
       json!({
         "type": "mark",
@@ -198,7 +245,7 @@ impl Performance {
     lsp_debug!("{},", msg);
     PerformanceMark {
       name: name.to_string(),
-      count: *count,
+      count,
       start: Instant::now(),
     }
   }
@@ -221,39 +268,32 @@ impl Performance {
     self.mark_inner(name, Some(args))
   }
 
+  /// Creates a performance mark which will be measured against on drop. Use
+  /// like this:
+  /// ```rust
+  /// let _mark = self.performance.measure_scope("foo");
+  /// ```
+  /// Don't use like this:
+  /// ```rust
+  /// // ❌
+  /// let _ = self.performance.measure_scope("foo");
+  /// ```
+  pub fn measure_scope<S: AsRef<str>>(&self, name: S) -> PerformanceScopeMark {
+    PerformanceScopeMark {
+      performance_inner: self.0.clone(),
+      inner: Some(self.mark(name)),
+    }
+  }
+
   /// A function which accepts a previously created performance mark which will
   /// be used to finalize the duration of the span being measured, and add the
   /// measurement to the internal buffer.
   pub fn measure(&self, mark: PerformanceMark) -> Duration {
-    let measure = PerformanceMeasure::from(mark);
+    self.0.lock().measure(mark)
-    lsp_debug!(
-      "{},",
-      json!({
-        "type": "measure",
-        "name": measure.name,
-        "count": measure.count,
-        "duration": measure.duration.as_micros() as f64 / 1000.0,
-      })
-    );
-    let duration = measure.duration;
-    {
-      let mut measurements_by_type = self.measurements_by_type.lock();
-      let measurement = measurements_by_type
-        .entry(measure.name.to_string())
-        .or_insert((0, 0.0));
-      measurement.1 += duration.as_micros() as f64 / 1000.0;
-    }
-    let mut measures = self.measures.lock();
-    measures.push_front(measure);
-    while measures.len() > self.max_size {
-      measures.pop_back();
-    }
-    duration
   }
 
   pub fn to_vec(&self) -> Vec<PerformanceMeasure> {
-    let measures = self.measures.lock();
+    self.0.lock().measures.iter().cloned().collect()
-    measures.iter().cloned().collect()
   }
 }