fix(lsp): fix deadlocks, use one big mutex (#9271)

The LSP code had numerous places where competing threads could take out
out locks in different orders, making it very prone to deadlocks.
This commit sidesteps the entire issue by switching to a single lock.

The above is a little white lie: the Sources struct still uses a mutex
internally to avoid having to boil the ocean (because being honest about
what it does involves changing all its methods to `&mut self` but that
ripples out extensively...) I'll save that one for another day.

1 files changed, 67 insertions, 20 deletions

diff --git a/cli/lsp/sources.rs b/cli/lsp/sources.rs
index fac1120fb..fc09f3f4d 100644
--- a/cli/lsp/sources.rs
+++ b/cli/lsp/sources.rs
@@ -51,7 +51,10 @@ struct Metadata {
 }
 
 #[derive(Debug, Clone, Default)]
-pub struct Sources {
+pub struct Sources(Arc<Mutex<Inner>>);
+
+#[derive(Debug, Default)]
+struct Inner {
   http_cache: HttpCache,
   maybe_import_map: Option<ImportMap>,
   metadata: HashMap<ModuleSpecifier, Metadata>,
@@ -61,13 +64,64 @@ pub struct Sources {
 
 impl Sources {
   pub fn new(location: &Path) -> Self {
+    Self(Arc::new(Mutex::new(Inner::new(location))))
+  }
+
+  pub fn contains(&self, specifier: &ModuleSpecifier) -> bool {
+    self.0.lock().unwrap().contains(specifier)
+  }
+
+  /// Provides the length of the source content, calculated in a way that should
+  /// match the behavior of JavaScript, where strings are stored effectively as
+  /// `&[u16]` and when counting "chars" we need to represent the string as a
+  /// UTF-16 string in Rust.
+  pub fn get_length_utf16(&self, specifier: &ModuleSpecifier) -> Option<usize> {
+    self.0.lock().unwrap().get_length_utf16(specifier)
+  }
+
+  pub fn get_line_index(
+    &self,
+    specifier: &ModuleSpecifier,
+  ) -> Option<LineIndex> {
+    self.0.lock().unwrap().get_line_index(specifier)
+  }
+
+  pub fn get_media_type(
+    &self,
+    specifier: &ModuleSpecifier,
+  ) -> Option<MediaType> {
+    self.0.lock().unwrap().get_media_type(specifier)
+  }
+
+  pub fn get_script_version(
+    &self,
+    specifier: &ModuleSpecifier,
+  ) -> Option<String> {
+    self.0.lock().unwrap().get_script_version(specifier)
+  }
+
+  pub fn get_text(&self, specifier: &ModuleSpecifier) -> Option<String> {
+    self.0.lock().unwrap().get_text(specifier)
+  }
+
+  pub fn resolve_import(
+    &self,
+    specifier: &str,
+    referrer: &ModuleSpecifier,
+  ) -> Option<(ModuleSpecifier, MediaType)> {
+    self.0.lock().unwrap().resolve_import(specifier, referrer)
+  }
+}
+
+impl Inner {
+  fn new(location: &Path) -> Self {
     Self {
       http_cache: HttpCache::new(location),
       ..Default::default()
     }
   }
 
-  pub fn contains(&mut self, specifier: &ModuleSpecifier) -> bool {
+  fn contains(&mut self, specifier: &ModuleSpecifier) -> bool {
     if let Some(specifier) = self.resolve_specifier(specifier) {
       if self.get_metadata(&specifier).is_some() {
         return true;
@@ -76,20 +130,13 @@ impl Sources {
     false
   }
 
-  /// Provides the length of the source content, calculated in a way that should
-  /// match the behavior of JavaScript, where strings are stored effectively as
-  /// `&[u16]` and when counting "chars" we need to represent the string as a
-  /// UTF-16 string in Rust.
-  pub fn get_length_utf16(
-    &mut self,
-    specifier: &ModuleSpecifier,
-  ) -> Option<usize> {
+  fn get_length_utf16(&mut self, specifier: &ModuleSpecifier) -> Option<usize> {
     let specifier = self.resolve_specifier(specifier)?;
     let metadata = self.get_metadata(&specifier)?;
     Some(metadata.source.encode_utf16().count())
   }
 
-  pub fn get_line_index(
+  fn get_line_index(
     &mut self,
     specifier: &ModuleSpecifier,
   ) -> Option<LineIndex> {
@@ -98,7 +145,7 @@ impl Sources {
     Some(metadata.line_index)
   }
 
-  pub fn get_media_type(
+  fn get_media_type(
     &mut self,
     specifier: &ModuleSpecifier,
   ) -> Option<MediaType> {
@@ -236,7 +283,7 @@ impl Sources {
     None
   }
 
-  pub fn get_script_version(
+  fn get_script_version(
     &mut self,
     specifier: &ModuleSpecifier,
   ) -> Option<String> {
@@ -257,7 +304,7 @@ impl Sources {
     None
   }
 
-  pub fn get_text(&mut self, specifier: &ModuleSpecifier) -> Option<String> {
+  fn get_text(&mut self, specifier: &ModuleSpecifier) -> Option<String> {
     let specifier = self.resolve_specifier(specifier)?;
     let metadata = self.get_metadata(&specifier)?;
     Some(metadata.source)
@@ -277,7 +324,7 @@ impl Sources {
     Some((resolved_specifier, media_type))
   }
 
-  pub fn resolve_import(
+  fn resolve_import(
     &mut self,
     specifier: &str,
     referrer: &ModuleSpecifier,
@@ -373,7 +420,7 @@ mod tests {
 
   #[test]
   fn test_sources_get_script_version() {
-    let (mut sources, _) = setup();
+    let (sources, _) = setup();
     let c = PathBuf::from(env::var_os("CARGO_MANIFEST_DIR").unwrap());
     let tests = c.join("tests");
     let specifier = ModuleSpecifier::resolve_path(
@@ -386,7 +433,7 @@ mod tests {
 
   #[test]
   fn test_sources_get_text() {
-    let (mut sources, _) = setup();
+    let (sources, _) = setup();
     let c = PathBuf::from(env::var_os("CARGO_MANIFEST_DIR").unwrap());
     let tests = c.join("tests");
     let specifier = ModuleSpecifier::resolve_path(
@@ -401,7 +448,7 @@ mod tests {
 
   #[test]
   fn test_sources_get_length_utf16() {
-    let (mut sources, _) = setup();
+    let (sources, _) = setup();
     let c = PathBuf::from(env::var_os("CARGO_MANIFEST_DIR").unwrap());
     let tests = c.join("tests");
     let specifier = ModuleSpecifier::resolve_path(
@@ -416,10 +463,10 @@ mod tests {
 
   #[test]
   fn test_sources_resolve_specifier_non_supported_schema() {
-    let (mut sources, _) = setup();
+    let (sources, _) = setup();
     let specifier = ModuleSpecifier::resolve_url("foo://a/b/c.ts")
       .expect("could not create specifier");
-    let actual = sources.resolve_specifier(&specifier);
+    let actual = sources.0.lock().unwrap().resolve_specifier(&specifier);
     assert!(actual.is_none());
   }
 }