Address review comments

This commit is contained in:
Jonas Schievink 2021-07-12 15:19:53 +02:00
parent abe0ead3a2
commit 29db33ce76
2 changed files with 30 additions and 22 deletions

View File

@ -15,7 +15,7 @@ use std::{
ffi::OsStr, ffi::OsStr,
io, io,
path::{Path, PathBuf}, path::{Path, PathBuf},
sync::Arc, sync::{Arc, Mutex},
}; };
use tt::{SmolStr, Subtree}; use tt::{SmolStr, Subtree};
@ -27,7 +27,7 @@ pub use version::{read_dylib_info, RustCInfo};
#[derive(Debug, Clone)] #[derive(Debug, Clone)]
struct ProcMacroProcessExpander { struct ProcMacroProcessExpander {
process: Arc<ProcMacroProcessSrv>, process: Arc<Mutex<ProcMacroProcessSrv>>,
dylib_path: PathBuf, dylib_path: PathBuf,
name: SmolStr, 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(), 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) Ok(result.expansion)
} }
} }
#[derive(Debug)] #[derive(Debug)]
pub struct ProcMacroClient { 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 { impl ProcMacroClient {
@ -73,7 +83,7 @@ impl ProcMacroClient {
args: impl IntoIterator<Item = impl AsRef<OsStr>>, args: impl IntoIterator<Item = impl AsRef<OsStr>>,
) -> io::Result<ProcMacroClient> { ) -> io::Result<ProcMacroClient> {
let process = ProcMacroProcessSrv::run(process_path, args)?; 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> { 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) => { Err(err) => {
eprintln!("Failed to find proc macros. Error: {:#?}", err); eprintln!("Failed to find proc macros. Error: {:#?}", err);
return vec![]; return vec![];

View File

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