Merge #4119

4119: Cache proc-macro dlls r=matklad a=edwin0cheng

This PR try to fix a deadlock in proc-macro srv by not unloading dlls.

Currently we load and unload dlls for each request, however rustc TLS is leaky , such that if we do it a lot of times, all TLS index will be consumed and it will be deadlocked inside panic (it is because panic itself is using TLS too).


Co-authored-by: Edwin Cheng <[email protected]>

4 files changed, 110 insertions, 65 deletions

diff --git a/crates/ra_proc_macro_srv/src/cli.rs b/crates/ra_proc_macro_srv/src/cli.rs
index 7282e5b9b..1437794c9 100644
--- a/crates/ra_proc_macro_srv/src/cli.rs
+++ b/crates/ra_proc_macro_srv/src/cli.rs
@@ -1,15 +1,17 @@
 //! Driver for proc macro server
 
-use crate::{expand_task, list_macros};
+use crate::ProcMacroSrv;
 use ra_proc_macro::msg::{self, Message};
 use std::io;
 
 pub fn run() -> io::Result<()> {
+    let mut srv = ProcMacroSrv::default();
+
     while let Some(req) = read_request()? {
         let res = match req {
-            msg::Request::ListMacro(task) => Ok(msg::Response::ListMacro(list_macros(&task))),
+            msg::Request::ListMacro(task) => srv.list_macros(&task).map(msg::Response::ListMacro),
             msg::Request::ExpansionMacro(task) => {
-                expand_task(&task).map(msg::Response::ExpansionMacro)
+                srv.expand(&task).map(msg::Response::ExpansionMacro)
             }
         };
 
diff --git a/crates/ra_proc_macro_srv/src/dylib.rs b/crates/ra_proc_macro_srv/src/dylib.rs
index d202eb0fd..aa84e951c 100644
--- a/crates/ra_proc_macro_srv/src/dylib.rs
+++ b/crates/ra_proc_macro_srv/src/dylib.rs
@@ -2,13 +2,12 @@
 
 use crate::{proc_macro::bridge, rustc_server::TokenStream};
 use std::fs::File;
-use std::path::Path;
+use std::path::{Path, PathBuf};
 
 use goblin::{mach::Mach, Object};
 use libloading::Library;
 use memmap::Mmap;
 use ra_proc_macro::ProcMacroKind;
-
 use std::io;
 
 const NEW_REGISTRAR_SYMBOL: &str = "_rustc_proc_macro_decls_";
@@ -109,23 +108,21 @@ impl ProcMacroLibraryLibloading {
     }
 }
 
-type ProcMacroLibraryImpl = ProcMacroLibraryLibloading;
-
 pub struct Expander {
-    libs: Vec<ProcMacroLibraryImpl>,
+    inner: ProcMacroLibraryLibloading,
 }
 
 impl Expander {
-    pub fn new(lib: &Path) -> Result<Expander, String> {
+    pub fn new(lib: &Path) -> io::Result<Expander> {
         // Some libraries for dynamic loading require canonicalized path even when it is
         // already absolute
-        let lib = lib
-            .canonicalize()
-            .unwrap_or_else(|err| panic!("Cannot canonicalize {}: {:?}", lib.display(), err));
+        let lib = lib.canonicalize()?;
+
+        let lib = ensure_file_with_lock_free_access(&lib)?;
 
-        let library = ProcMacroLibraryImpl::open(&lib).map_err(|e| e.to_string())?;
+        let library = ProcMacroLibraryLibloading::open(&lib)?;
 
-        Ok(Expander { libs: vec![library] })
+        Ok(Expander { inner: library })
     }
 
     pub fn expand(
@@ -141,38 +138,36 @@ impl Expander {
                 TokenStream::with_subtree(attr.clone())
             });
 
-        for lib in &self.libs {
-            for proc_macro in &lib.exported_macros {
-                match proc_macro {
-                    bridge::client::ProcMacro::CustomDerive { trait_name, client, .. }
-                        if *trait_name == macro_name =>
-                    {
-                        let res = client.run(
-                            &crate::proc_macro::bridge::server::SameThread,
-                            crate::rustc_server::Rustc::default(),
-                            parsed_body,
-                        );
-                        return res.map(|it| it.subtree);
-                    }
-                    bridge::client::ProcMacro::Bang { name, client } if *name == macro_name => {
-                        let res = client.run(
-                            &crate::proc_macro::bridge::server::SameThread,
-                            crate::rustc_server::Rustc::default(),
-                            parsed_body,
-                        );
-                        return res.map(|it| it.subtree);
-                    }
-                    bridge::client::ProcMacro::Attr { name, client } if *name == macro_name => {
-                        let res = client.run(
-                            &crate::proc_macro::bridge::server::SameThread,
-                            crate::rustc_server::Rustc::default(),
-                            parsed_attributes,
-                            parsed_body,
-                        );
-                        return res.map(|it| it.subtree);
-                    }
-                    _ => continue,
+        for proc_macro in &self.inner.exported_macros {
+            match proc_macro {
+                bridge::client::ProcMacro::CustomDerive { trait_name, client, .. }
+                    if *trait_name == macro_name =>
+                {
+                    let res = client.run(
+                        &crate::proc_macro::bridge::server::SameThread,
+                        crate::rustc_server::Rustc::default(),
+                        parsed_body,
+                    );
+                    return res.map(|it| it.subtree);
+                }
+                bridge::client::ProcMacro::Bang { name, client } if *name == macro_name => {
+                    let res = client.run(
+                        &crate::proc_macro::bridge::server::SameThread,
+                        crate::rustc_server::Rustc::default(),
+                        parsed_body,
+                    );
+                    return res.map(|it| it.subtree);
+                }
+                bridge::client::ProcMacro::Attr { name, client } if *name == macro_name => {
+                    let res = client.run(
+                        &crate::proc_macro::bridge::server::SameThread,
+                        crate::rustc_server::Rustc::default(),
+                        parsed_attributes,
+                        parsed_body,
+                    );
+                    return res.map(|it| it.subtree);
                 }
+                _ => continue,
             }
         }
 
@@ -180,9 +175,9 @@ impl Expander {
     }
 
     pub fn list_macros(&self) -> Vec<(String, ProcMacroKind)> {
-        self.libs
+        self.inner
+            .exported_macros
             .iter()
-            .flat_map(|it| &it.exported_macros)
             .map(|proc_macro| match proc_macro {
                 bridge::client::ProcMacro::CustomDerive { trait_name, .. } => {
                     (trait_name.to_string(), ProcMacroKind::CustomDerive)
@@ -197,3 +192,33 @@ impl Expander {
             .collect()
     }
 }
+
+/// Copy the dylib to temp directory to prevent locking in Windows
+#[cfg(windows)]
+fn ensure_file_with_lock_free_access(path: &Path) -> io::Result<PathBuf> {
+    use std::{ffi::OsString, time::SystemTime};
+
+    let mut to = std::env::temp_dir();
+
+    let file_name = path.file_name().ok_or_else(|| {
+        io::Error::new(
+            io::ErrorKind::InvalidInput,
+            format!("File path is invalid: {}", path.display()),
+        )
+    })?;
+
+    // generate a time deps unique number
+    let t = SystemTime::now().duration_since(std::time::UNIX_EPOCH).expect("Time went backwards");
+
+    let mut unique_name = OsString::from(t.as_millis().to_string());
+    unique_name.push(file_name);
+
+    to.push(unique_name);
+    std::fs::copy(path, &to).unwrap();
+    Ok(to)
+}
+
+#[cfg(unix)]
+fn ensure_file_with_lock_free_access(path: &Path) -> io::Result<PathBuf> {
+    Ok(path.to_path_buf())
+}
diff --git a/crates/ra_proc_macro_srv/src/lib.rs b/crates/ra_proc_macro_srv/src/lib.rs
index 3aca859db..922bb84bb 100644
--- a/crates/ra_proc_macro_srv/src/lib.rs
+++ b/crates/ra_proc_macro_srv/src/lib.rs
@@ -21,28 +21,46 @@ mod dylib;
 
 use proc_macro::bridge::client::TokenStream;
 use ra_proc_macro::{ExpansionResult, ExpansionTask, ListMacrosResult, ListMacrosTask};
-use std::path::Path;
+use std::{
+    collections::{hash_map::Entry, HashMap},
+    fs,
+    path::{Path, PathBuf},
+    time::SystemTime,
+};
 
-pub(crate) fn expand_task(task: &ExpansionTask) -> Result<ExpansionResult, String> {
-    let expander = create_expander(&task.lib);
+#[derive(Default)]
+pub(crate) struct ProcMacroSrv {
+    expanders: HashMap<(PathBuf, SystemTime), dylib::Expander>,
+}
 
-    match expander.expand(&task.macro_name, &task.macro_body, task.attributes.as_ref()) {
-        Ok(expansion) => Ok(ExpansionResult { expansion }),
-        Err(msg) => {
-            Err(format!("Cannot perform expansion for {}: error {:?}", &task.macro_name, msg))
+impl ProcMacroSrv {
+    pub fn expand(&mut self, task: &ExpansionTask) -> Result<ExpansionResult, String> {
+        let expander = self.expander(&task.lib)?;
+        match expander.expand(&task.macro_name, &task.macro_body, task.attributes.as_ref()) {
+            Ok(expansion) => Ok(ExpansionResult { expansion }),
+            Err(msg) => {
+                Err(format!("Cannot perform expansion for {}: error {:?}", &task.macro_name, msg))
+            }
         }
     }
-}
 
-pub(crate) fn list_macros(task: &ListMacrosTask) -> ListMacrosResult {
-    let expander = create_expander(&task.lib);
+    pub fn list_macros(&mut self, task: &ListMacrosTask) -> Result<ListMacrosResult, String> {
+        let expander = self.expander(&task.lib)?;
+        Ok(ListMacrosResult { macros: expander.list_macros() })
+    }
 
-    ListMacrosResult { macros: expander.list_macros() }
-}
+    fn expander(&mut self, path: &Path) -> Result<&dylib::Expander, String> {
+        let time = fs::metadata(path).and_then(|it| it.modified()).map_err(|err| {
+            format!("Failed to get file metadata for {}: {:?}", path.display(), err)
+        })?;
 
-fn create_expander(lib: &Path) -> dylib::Expander {
-    dylib::Expander::new(lib)
-        .unwrap_or_else(|err| panic!("Cannot create expander for {}: {:?}", lib.display(), err))
+        Ok(match self.expanders.entry((path.to_path_buf(), time)) {
+            Entry::Vacant(v) => v.insert(dylib::Expander::new(path).map_err(|err| {
+                format!("Cannot create expander for {}: {:?}", path.display(), err)
+            })?),
+            Entry::Occupied(e) => e.into_mut(),
+        })
+    }
 }
 
 pub mod cli;
diff --git a/crates/ra_proc_macro_srv/src/tests/utils.rs b/crates/ra_proc_macro_srv/src/tests/utils.rs
index 2139ec7a4..646a427c5 100644
--- a/crates/ra_proc_macro_srv/src/tests/utils.rs
+++ b/crates/ra_proc_macro_srv/src/tests/utils.rs
@@ -1,7 +1,7 @@
 //! utils used in proc-macro tests
 
 use crate::dylib;
-use crate::list_macros;
+use crate::ProcMacroSrv;
 pub use difference::Changeset as __Changeset;
 use ra_proc_macro::ListMacrosTask;
 use std::str::FromStr;
@@ -59,7 +59,7 @@ pub fn assert_expand(
 pub fn list(crate_name: &str, version: &str) -> Vec<String> {
     let path = fixtures::dylib_path(crate_name, version);
     let task = ListMacrosTask { lib: path };
-
-    let res = list_macros(&task);
+    let mut srv = ProcMacroSrv::default();
+    let res = srv.list_macros(&task).unwrap();
     res.macros.into_iter().map(|(name, kind)| format!("{} [{:?}]", name, kind)).collect()
 }