From 5a9ca311e36eb6f336f0a8f24df842bdca824388 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Thu, 8 Jul 2021 16:40:14 +0200 Subject: [PATCH 1/3] Remove proc macro management thread --- crates/proc_macro_api/src/lib.rs | 8 +- crates/proc_macro_api/src/process.rs | 106 ++++++++++----------------- crates/stdx/src/lib.rs | 1 + 3 files changed, 44 insertions(+), 71 deletions(-) diff --git a/crates/proc_macro_api/src/lib.rs b/crates/proc_macro_api/src/lib.rs index 654bd9943eb..5054bc7643d 100644 --- a/crates/proc_macro_api/src/lib.rs +++ b/crates/proc_macro_api/src/lib.rs @@ -20,7 +20,7 @@ use std::{ use tt::{SmolStr, Subtree}; -use crate::process::{ProcMacroProcessSrv, ProcMacroProcessThread}; +use crate::process::ProcMacroProcessSrv; pub use rpc::{ExpansionResult, ExpansionTask, ListMacrosResult, ListMacrosTask, ProcMacroKind}; pub use version::{read_dylib_info, RustCInfo}; @@ -64,16 +64,16 @@ impl base_db::ProcMacroExpander for ProcMacroProcessExpander { #[derive(Debug)] pub struct ProcMacroClient { process: Arc, - thread: ProcMacroProcessThread, } impl ProcMacroClient { + /// Spawns an external process as the proc macro server and returns a client connected to it. pub fn extern_process( process_path: PathBuf, args: impl IntoIterator>, ) -> io::Result { - let (thread, process) = ProcMacroProcessSrv::run(process_path, args)?; - Ok(ProcMacroClient { process: Arc::new(process), thread }) + let process = ProcMacroProcessSrv::run(process_path, args)?; + Ok(ProcMacroClient { process: Arc::new(process) }) } pub fn by_dylib_path(&self, dylib_path: &Path) -> Vec { diff --git a/crates/proc_macro_api/src/process.rs b/crates/proc_macro_api/src/process.rs index 91d6a3811be..592c1282c0b 100644 --- a/crates/proc_macro_api/src/process.rs +++ b/crates/proc_macro_api/src/process.rs @@ -3,13 +3,13 @@ use std::{ convert::{TryFrom, TryInto}, ffi::{OsStr, OsString}, + fmt, io::{self, BufRead, BufReader, Write}, path::{Path, PathBuf}, - process::{Child, Command, Stdio}, - sync::{Arc, Weak}, + process::{Child, ChildStdin, ChildStdout, Command, Stdio}, + sync::Mutex, }; -use crossbeam_channel::{bounded, Receiver, Sender}; use stdx::JodChild; use crate::{ @@ -17,38 +17,31 @@ use crate::{ rpc::{ListMacrosResult, ListMacrosTask, ProcMacroKind}, }; -#[derive(Debug, Default)] pub(crate) struct ProcMacroProcessSrv { - inner: Weak>, + process: Mutex, + stdio: Mutex<(ChildStdin, BufReader)>, } -#[derive(Debug)] -pub(crate) struct ProcMacroProcessThread { - // XXX: drop order is significant - sender: Arc>, - handle: jod_thread::JoinHandle<()>, +impl fmt::Debug for ProcMacroProcessSrv { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("ProcMacroProcessSrv").field("process", &self.process).finish() + } } impl ProcMacroProcessSrv { pub(crate) fn run( process_path: PathBuf, args: impl IntoIterator>, - ) -> io::Result<(ProcMacroProcessThread, ProcMacroProcessSrv)> { - let process = Process::run(process_path, args)?; + ) -> io::Result { + let mut process = Process::run(process_path, args)?; + let (stdin, stdout) = process.stdio().expect("couldn't access child stdio"); - let (task_tx, task_rx) = bounded(0); - let handle = jod_thread::Builder::new() - .name("ProcMacroClient".to_owned()) - .spawn(move || { - client_loop(task_rx, process); - }) - .expect("failed to spawn thread"); + let srv = ProcMacroProcessSrv { + process: Mutex::new(process), + stdio: Mutex::new((stdin, stdout)), + }; - let task_tx = Arc::new(task_tx); - let srv = ProcMacroProcessSrv { inner: Arc::downgrade(&task_tx) }; - let thread = ProcMacroProcessThread { handle, sender: task_tx }; - - Ok((thread, srv)) + Ok(srv) } pub(crate) fn find_proc_macros( @@ -65,18 +58,27 @@ impl ProcMacroProcessSrv { where R: TryFrom, { - let (result_tx, result_rx) = bounded(0); - let sender = match self.inner.upgrade() { - None => return Err(tt::ExpansionError::Unknown("proc macro process is closed".into())), - Some(it) => it, - }; - sender - .send(Task { req, result_tx }) - .map_err(|_| tt::ExpansionError::Unknown("proc macro server crashed".into()))?; + 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 res = result_rx - .recv() - .map_err(|_| tt::ExpansionError::Unknown("proc macro server crashed".into()))?; + let mut buf = String::new(); + let res = match send_request(stdin, stdout, req, &mut buf) { + Ok(res) => res, + Err(err) => { + let mut process = self.process.lock().unwrap_or_else(|e| e.into_inner()); + log::error!( + "proc macro server crashed, server process state: {:?}, server request error: {:?}", + process.child.try_wait(), + err + ); + let res = Response::Error(ResponseError { + code: ErrorCode::ServerErrorEnd, + message: "proc macro server crashed".into(), + }); + Some(res) + } + }; match res { Some(Response::Error(err)) => Err(tt::ExpansionError::ExpansionError(err.message)), @@ -88,37 +90,7 @@ impl ProcMacroProcessSrv { } } -fn client_loop(task_rx: Receiver, mut process: Process) { - let (mut stdin, mut stdout) = process.stdio().expect("couldn't access child stdio"); - - let mut buf = String::new(); - - for Task { req, result_tx } in task_rx { - match send_request(&mut stdin, &mut stdout, req, &mut buf) { - Ok(res) => result_tx.send(res).unwrap(), - Err(err) => { - log::error!( - "proc macro server crashed, server process state: {:?}, server request error: {:?}", - process.child.try_wait(), - err - ); - let res = Response::Error(ResponseError { - code: ErrorCode::ServerErrorEnd, - message: "proc macro server crashed".into(), - }); - result_tx.send(res.into()).unwrap(); - // Exit the thread. - break; - } - } - } -} - -struct Task { - req: Request, - result_tx: Sender>, -} - +#[derive(Debug)] struct Process { child: JodChild, } @@ -133,7 +105,7 @@ impl Process { Ok(Process { child }) } - fn stdio(&mut self) -> Option<(impl Write, impl BufRead)> { + fn stdio(&mut self) -> Option<(ChildStdin, BufReader)> { let stdin = self.child.stdin.take()?; let stdout = self.child.stdout.take()?; let read = BufReader::new(stdout); diff --git a/crates/stdx/src/lib.rs b/crates/stdx/src/lib.rs index 2963484faef..e83d5db437d 100644 --- a/crates/stdx/src/lib.rs +++ b/crates/stdx/src/lib.rs @@ -111,6 +111,7 @@ pub fn defer(f: F) -> impl Drop { } #[cfg_attr(not(target_arch = "wasm32"), repr(transparent))] +#[derive(Debug)] pub struct JodChild(pub std::process::Child); impl ops::Deref for JodChild { From abe0ead3a2896ebd4eb47873b0ed1084258360d1 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Thu, 8 Jul 2021 17:10:35 +0200 Subject: [PATCH 2/3] Use `#[derive(Debug)]` --- crates/proc_macro_api/src/process.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/crates/proc_macro_api/src/process.rs b/crates/proc_macro_api/src/process.rs index 592c1282c0b..bb4ea05d6f1 100644 --- a/crates/proc_macro_api/src/process.rs +++ b/crates/proc_macro_api/src/process.rs @@ -3,7 +3,6 @@ use std::{ convert::{TryFrom, TryInto}, ffi::{OsStr, OsString}, - fmt, io::{self, BufRead, BufReader, Write}, path::{Path, PathBuf}, process::{Child, ChildStdin, ChildStdout, Command, Stdio}, @@ -17,17 +16,12 @@ use crate::{ rpc::{ListMacrosResult, ListMacrosTask, ProcMacroKind}, }; +#[derive(Debug)] pub(crate) struct ProcMacroProcessSrv { process: Mutex, stdio: Mutex<(ChildStdin, BufReader)>, } -impl fmt::Debug for ProcMacroProcessSrv { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("ProcMacroProcessSrv").field("process", &self.process).finish() - } -} - impl ProcMacroProcessSrv { pub(crate) fn run( process_path: PathBuf, From 29db33ce76414913eba6d4851f990caf3df14465 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Mon, 12 Jul 2021 15:19:53 +0200 Subject: [PATCH 3/3] Address review comments --- crates/proc_macro_api/src/lib.rs | 27 +++++++++++++++++++++------ crates/proc_macro_api/src/process.rs | 25 +++++++++---------------- 2 files changed, 30 insertions(+), 22 deletions(-) diff --git a/crates/proc_macro_api/src/lib.rs b/crates/proc_macro_api/src/lib.rs index 5054bc7643d..244f65579ed 100644 --- a/crates/proc_macro_api/src/lib.rs +++ b/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, + process: Arc>, 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, + /// 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>, } impl ProcMacroClient { @@ -73,7 +83,7 @@ impl ProcMacroClient { args: impl IntoIterator>, ) -> io::Result { 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 { @@ -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![]; diff --git a/crates/proc_macro_api/src/process.rs b/crates/proc_macro_api/src/process.rs index bb4ea05d6f1..6222dd649af 100644 --- a/crates/proc_macro_api/src/process.rs +++ b/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, - stdio: Mutex<(ChildStdin, BufReader)>, + process: Process, + stdin: ChildStdin, + stdout: BufReader, } 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, 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(&self, req: Request) -> Result + pub(crate) fn send_task(&mut self, req: Request) -> Result where R: TryFrom, { - 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 {