Address review comments

Author: jonas-schievink

crates/proc_macro_api/src/lib.rs

@@ -15,7 +15,7 @@ use std::{
     ffi::OsStr,
     io,
     path::{Path, PathBuf},
-    sync::Arc,
+    sync::{Arc, Mutex},
 };
 
 use tt::{SmolStr, Subtree};
@@ -27,7 +27,7 @@ pub use version::{read_dylib_info, RustCInfo};
 
 #[derive(Debug, Clone)]
 struct ProcMacroProcessExpander {
-    process: Arc<ProcMacroProcessSrv>,
+    process: Arc<Mutex<ProcMacroProcessSrv>>,
     dylib_path: PathBuf,
     name: SmolStr,
 }
@@ -56,14 +56,24 @@ impl base_db::ProcMacroExpander for ProcMacroProcessExpander {
             env: env.iter().map(|(k, v)| (k.to_string(), v.to_string())).collect(),
         };
 
-        let result: ExpansionResult = self.process.send_task(msg::Request::ExpansionMacro(task))?;
+        let result: ExpansionResult = self
+            .process
+            .lock()
+            .unwrap_or_else(|e| e.into_inner())
+            .send_task(msg::Request::ExpansionMacro(task))?;
         Ok(result.expansion)
     }
 }
 
 #[derive(Debug)]
 pub struct ProcMacroClient {
-    process: Arc<ProcMacroProcessSrv>,
+    /// Currently, the proc macro process expands all procedural macros sequentially.
+    ///
+    /// That means that concurrent salsa requests may block each other when expanding proc macros,
+    /// which is unfortunate, but simple and good enough for the time being.
+    ///
+    /// Therefore, we just wrap the `ProcMacroProcessSrv` in a mutex here.
+    process: Arc<Mutex<ProcMacroProcessSrv>>,
 }
 
 impl ProcMacroClient {
@@ -73,7 +83,7 @@ impl ProcMacroClient {
         args: impl IntoIterator<Item = impl AsRef<OsStr>>,
     ) -> io::Result<ProcMacroClient> {
         let process = ProcMacroProcessSrv::run(process_path, args)?;
-        Ok(ProcMacroClient { process: Arc::new(process) })
+        Ok(ProcMacroClient { process: Arc::new(Mutex::new(process)) })
     }
 
     pub fn by_dylib_path(&self, dylib_path: &Path) -> Vec<ProcMacro> {
@@ -93,7 +103,12 @@ impl ProcMacroClient {
             }
         }
 
-        let macros = match self.process.find_proc_macros(dylib_path) {
+        let macros = match self
+            .process
+            .lock()
+            .unwrap_or_else(|e| e.into_inner())
+            .find_proc_macros(dylib_path)
+        {
             Err(err) => {
                 eprintln!("Failed to find proc macros. Error: {:#?}", err);
                 return vec![];

crates/proc_macro_api/src/process.rs

@@ -6,7 +6,6 @@ use std::{
     io::{self, BufRead, BufReader, Write},
     path::{Path, PathBuf},
     process::{Child, ChildStdin, ChildStdout, Command, Stdio},
-    sync::Mutex,
 };
 
 use stdx::JodChild;
@@ -18,8 +17,9 @@ use crate::{
 
 #[derive(Debug)]
 pub(crate) struct ProcMacroProcessSrv {
-    process: Mutex<Process>,
-    stdio: Mutex<(ChildStdin, BufReader<ChildStdout>)>,
+    process: Process,
+    stdin: ChildStdin,
+    stdout: BufReader<ChildStdout>,
 }
 
 impl ProcMacroProcessSrv {
@@ -30,16 +30,13 @@ impl ProcMacroProcessSrv {
         let mut process = Process::run(process_path, args)?;
         let (stdin, stdout) = process.stdio().expect("couldn't access child stdio");
 
-        let srv = ProcMacroProcessSrv {
-            process: Mutex::new(process),
-            stdio: Mutex::new((stdin, stdout)),
-        };
+        let srv = ProcMacroProcessSrv { process, stdin, stdout };
 
         Ok(srv)
     }
 
     pub(crate) fn find_proc_macros(
-        &self,
+        &mut self,
         dylib_path: &Path,
     ) -> Result<Vec<(String, ProcMacroKind)>, tt::ExpansionError> {
         let task = ListMacrosTask { lib: dylib_path.to_path_buf() };
@@ -48,22 +45,18 @@ impl ProcMacroProcessSrv {
         Ok(result.macros)
     }
 
-    pub(crate) fn send_task<R>(&self, req: Request) -> Result<R, tt::ExpansionError>
+    pub(crate) fn send_task<R>(&mut self, req: Request) -> Result<R, tt::ExpansionError>
     where
         R: TryFrom<Response, Error = &'static str>,
     {
-        let mut guard = self.stdio.lock().unwrap_or_else(|e| e.into_inner());
-        let stdio = &mut *guard;
-        let (stdin, stdout) = (&mut stdio.0, &mut stdio.1);
-
         let mut buf = String::new();
-        let res = match send_request(stdin, stdout, req, &mut buf) {
+        let res = match send_request(&mut self.stdin, &mut self.stdout, req, &mut buf) {
             Ok(res) => res,
             Err(err) => {
-                let mut process = self.process.lock().unwrap_or_else(|e| e.into_inner());
+                let result = self.process.child.try_wait();
                 log::error!(
                     "proc macro server crashed, server process state: {:?}, server request error: {:?}",
-                    process.child.try_wait(),
+                    result,
                     err
                 );
                 let res = Response::Error(ResponseError {