Merge #3754

3754: Use automatic thread joining for cargo-watch r=matklad a=matklad

r? @kiljacken 

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

Cargo.lock

@@ -563,9 +563,9 @@ dependencies = [
 
 [[package]]
 name = "jod-thread"
-version = "0.1.0"
+version = "0.1.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "2f52a11f73b88fab829a0e4d9e13ea5982c7ac457c72eb3541d82a4afdfce4ff"
+checksum = "4022656272c3e564a7cdebcaaba6518d844b0d0c1836597196efb5bfeb98bb49"
 
 [[package]]
 name = "kernel32-sys"
@@ -893,6 +893,7 @@ dependencies = [
  "cargo_metadata",
  "crossbeam-channel",
  "insta",
+ "jod-thread",
  "log",
  "lsp-types",
  "serde_json",

crates/ra_cargo_watch/Cargo.toml

@@ -10,6 +10,7 @@ lsp-types = { version = "0.73.0", features = ["proposed"] }
 log = "0.4.8"
 cargo_metadata = "0.9.1"
 serde_json = "1.0.48"
+jod-thread = "0.1.1"
 
 [dev-dependencies]
 insta = "0.15.0"

crates/ra_cargo_watch/src/lib.rs

@@ -12,7 +12,6 @@ use std::{
     io::{BufRead, BufReader},
     path::{Path, PathBuf},
     process::{Command, Stdio},
-    thread::JoinHandle,
     time::Instant,
 };
 
@@ -36,9 +35,10 @@ pub struct CheckOptions {
 /// The spawned thread is shut down when this struct is dropped.
 #[derive(Debug)]
 pub struct CheckWatcher {
-    pub task_recv: Receiver<CheckTask>,
+    // XXX: drop order is significant
     cmd_send: Option<Sender<CheckCommand>>,
-    handle: Option<JoinHandle<()>>,
+    handle: Option<jod_thread::JoinHandle<()>>,
+    pub task_recv: Receiver<CheckTask>,
 }
 
 impl CheckWatcher {
@@ -47,7 +47,7 @@ impl CheckWatcher {
 
         let (task_send, task_recv) = unbounded::<CheckTask>();
         let (cmd_send, cmd_recv) = unbounded::<CheckCommand>();
-        let handle = std::thread::spawn(move || {
+        let handle = jod_thread::spawn(move || {
             let mut check = CheckWatcherThread::new(options, workspace_root);
             check.run(&task_send, &cmd_recv);
         });
@@ -67,22 +67,6 @@ impl CheckWatcher {
     }
 }
 
-impl std::ops::Drop for CheckWatcher {
-    fn drop(&mut self) {
-        if let Some(handle) = self.handle.take() {
-            // Take the sender out of the option
-            let cmd_send = self.cmd_send.take();
-
-            // Dropping the sender finishes the thread loop
-            drop(cmd_send);
-
-            // Join the thread, it should finish shortly. We don't really care
-            // whether it panicked, so it is safe to ignore the result
-            let _ = handle.join();
-        }
-    }
-}
-
 #[derive(Debug)]
 pub enum CheckTask {
     /// Request a clearing of all cached diagnostics from the check watcher
@@ -237,8 +221,9 @@ pub struct DiagnosticWithFixes {
 /// The correct way to dispose of the thread is to drop it, on which the
 /// sub-process will be killed, and the thread will be joined.
 struct WatchThread {
-    handle: Option<JoinHandle<()>>,
+    // XXX: drop order is significant
     message_recv: Receiver<CheckEvent>,
+    _handle: Option<jod_thread::JoinHandle<()>>,
 }
 
 enum CheckEvent {
@@ -333,7 +318,7 @@ pub fn run_cargo(
 
 impl WatchThread {
     fn dummy() -> WatchThread {
-        WatchThread { handle: None, message_recv: never() }
+        WatchThread { message_recv: never(), _handle: None }
     }
 
     fn new(options: &CheckOptions, workspace_root: &Path) -> WatchThread {
@@ -352,7 +337,7 @@ impl WatchThread {
         let (message_send, message_recv) = unbounded();
         let workspace_root = workspace_root.to_owned();
         let handle = if options.enable {
-            Some(std::thread::spawn(move || {
+            Some(jod_thread::spawn(move || {
                 // If we trigger an error here, we will do so in the loop instead,
                 // which will break out of the loop, and continue the shutdown
                 let _ = message_send.send(CheckEvent::Begin);
@@ -383,23 +368,6 @@ impl WatchThread {
         } else {
             None
         };
-        WatchThread { handle, message_recv }
-    }
-}
-
-impl std::ops::Drop for WatchThread {
-    fn drop(&mut self) {
-        if let Some(handle) = self.handle.take() {
-            // Replace our reciever with dummy one, so we can drop and close the
-            // one actually communicating with the thread
-            let recv = std::mem::replace(&mut self.message_recv, never());
-
-            // Dropping the original reciever initiates thread sub-process shutdown
-            drop(recv);
-
-            // Join the thread, it should finish shortly. We don't really care
-            // whether it panicked, so it is safe to ignore the result
-            let _ = handle.join();
-        }
+        WatchThread { message_recv, _handle: handle }
     }
 }

crates/ra_prof/src/lib.rs

@@ -339,6 +339,14 @@ pub fn print_backtrace() {
     let bt = backtrace::Backtrace::new();
     eprintln!("{:?}", bt);
 }
+#[cfg(not(feature = "backtrace"))]
+pub fn print_backtrace() {
+    eprintln!(
+        r#"enable the backtrace feature:
+    ra_prof = {{ path = "../ra_prof", features = [ "backtrace"] }}
+"#
+    );
+}
 
 thread_local!(static IN_SCOPE: RefCell<bool> = RefCell::new(false));
 

crates/rust-analyzer/tests/heavy_tests/support.rs

@@ -83,9 +83,10 @@ pub fn project(fixture: &str) -> Server {
 pub struct Server {
     req_id: Cell<u64>,
     messages: RefCell<Vec<Message>>,
-    dir: TempDir,
     _thread: jod_thread::JoinHandle<()>,
     client: Connection,
+    /// XXX: remove the tempdir last
+    dir: TempDir,
 }
 
 impl Server {