Merge #687's head into fix/update-messages

crates/update/src/process.rs

@@ -2,7 +2,8 @@
 
 use std::ffi::OsStr;
 use std::path::{Path, PathBuf};
-use std::process::{Command, Stdio};
+use std::process::{Child, Command, Stdio};
+use std::sync::{Arc, Mutex, PoisonError};
 use std::time::{Duration, Instant};
 
 /// A `Command` that won't flash a console window on Windows, and clears AppImage-injected
@@ -115,18 +116,49 @@ fn read_all<R: std::io::Read + Send + 'static>(pipe: Option<R>) -> std::io::Resu
 }
 
 /// Starts `cmd` without waiting for it, and reaps it when it exits. The process keeps running
-/// when no thread can be started to reap it; only the wait is skipped.
+/// when no thread can be started to reap it. It is then reaped when a later call starts another
+/// process after it has exited; until then, on Unix, it stays in the process table.
 pub fn spawn_detached(mut cmd: Command) -> Result<(), String> {
-    let mut child = cmd.stdin(Stdio::null()).stdout(Stdio::null()).stderr(Stdio::null()).spawn().map_err(|e| format!("couldn't start it ({e})"))?;
-    let reaper = std::thread::Builder::new().name("reap-child".into()).spawn(move || {
-        let _ = child.wait();
-    });
-    if let Err(e) = reaper {
-        log::warn!("couldn't start a thread to wait for a started process ({e})");
-    }
+    let child = cmd.stdin(Stdio::null()).stdout(Stdio::null()).stderr(Stdio::null()).spawn().map_err(|e| format!("couldn't start it ({e})"))?;
+    reap_in_background(child, "a started process");
     Ok(())
 }
 
+/// Started processes that no thread waits for, because none could be started. On Unix a process
+/// that exits stays in the process table until it is waited for.
+static UNREAPED: Mutex<Vec<Child>> = Mutex::new(Vec::new());
+
+/// Waits for `child` on its own thread. When no thread can be started, `child` is kept in
+/// [`UNREAPED`]. Each call first waits for the kept processes that have exited, so a kept process
+/// is waited for at the first call after it exits.
+fn reap_in_background(child: Child, what: &str) {
+    reap_in_background_with(child, what, |wait| std::thread::Builder::new().name("reap-child".into()).spawn(wait).map(drop));
+}
+
+/// [`reap_in_background`] with `start_thread` in place of starting a thread (tests).
+fn reap_in_background_with(child: Child, what: &str, start_thread: impl FnOnce(Box<dyn FnOnce() + Send>) -> std::io::Result<()>) {
+    reap_exited();
+    let slot = Arc::new(Mutex::new(Some(child)));
+    let theirs = Arc::clone(&slot);
+    let wait = Box::new(move || {
+        let child = theirs.lock().unwrap_or_else(PoisonError::into_inner).take();
+        if let Some(mut child) = child {
+            let _ = child.wait();
+        }
+    });
+    if let Err(e) = start_thread(wait) {
+        log::warn!("couldn't start a thread to wait for {what} ({e})");
+        if let Some(child) = slot.lock().unwrap_or_else(PoisonError::into_inner).take() {
+            UNREAPED.lock().unwrap_or_else(PoisonError::into_inner).push(child);
+        }
+    }
+}
+
+/// Waits for the processes in [`UNREAPED`] that have exited, without blocking on the others.
+fn reap_exited() {
+    UNREAPED.lock().unwrap_or_else(PoisonError::into_inner).retain_mut(|child| matches!(child.try_wait(), Ok(None)));
+}
+
 /// Waits up to `timeout` for the process `pid` to exit, and says whether it has. A process that
 /// can't be opened, because it's gone or never was, counts as exited.
 #[cfg(windows)]
@@ -277,12 +309,7 @@ fn open_and_wait_success(target: &str, timeout: Duration) -> bool {
             Ok(Some(status)) => return status.success(),
             Ok(None) if start.elapsed() < timeout => std::thread::sleep(Duration::from_millis(25)),
             Ok(None) => {
-                let reaper = std::thread::Builder::new().name("reap-child".into()).spawn(move || {
-                    let _ = child.wait();
-                });
-                if let Err(e) = reaper {
-                    log::warn!("couldn't start a thread to wait for the opener of {target} ({e})");
-                }
+                reap_in_background(child, &format!("the opener of {target}"));
                 return true;
             }
             Err(_) => return false,
@@ -430,6 +457,38 @@ mod tests {
             assert_eq!(is_running(Path::new(&requested)), expected, "{requested}");
         }
     }
+
+    /// A process that no thread can wait for is kept, and waited for once it has exited, so it
+    /// does not stay in the process table.
+    #[test]
+    fn a_process_without_a_waiting_thread_is_reaped_later() {
+        let mut child = command("cat").stdin(Stdio::piped()).stdout(Stdio::piped()).stderr(Stdio::null()).spawn().unwrap();
+        let stdin = child.stdin.take().unwrap();
+        let mut stdout = child.stdout.take().unwrap();
+        let pid = child.id();
+        let kept = || UNREAPED.lock().unwrap_or_else(PoisonError::into_inner).iter().any(|c| c.id() == pid);
+        reap_in_background_with(child, "a test process", |_wait| Err(std::io::Error::other("no threads")));
+        assert!(kept(), "a running process with no thread to wait for it is kept");
+        // `cat` exits when its input closes, and its output closes as it exits.
+        drop(stdin);
+        let _ = std::io::copy(&mut stdout, &mut std::io::sink());
+        // `ps` shows an exited process that nothing has waited for in state Z, and nothing once it
+        // has been waited for (another test's call may do that first).
+        let state = || {
+            let out = std::process::Command::new("ps").args(["-o", "stat=", "-p", &pid.to_string()]).output().unwrap();
+            String::from_utf8_lossy(&out.stdout).trim().to_owned()
+        };
+        loop {
+            let now = state();
+            if now.is_empty() || now.starts_with('Z') {
+                break;
+            }
+            std::thread::yield_now();
+        }
+        reap_exited();
+        assert!(!kept(), "an exited process is waited for by the next call");
+        assert_eq!(state(), "", "the waited-for process has left the process table");
+    }
 }
 
 #[cfg(test)]

crates/update/src/self_update.rs

@@ -1012,10 +1012,23 @@ fn finish_step(
         record_done(paths, version);
     }
     let started = start(&exe);
-    if let Err(e) = &started {
+    let restarted = started.is_ok();
+    let result = with_restart(done, started);
+    if !restarted && let Err(e) = &result {
         record_failure(paths, e);
     }
-    done.and(started)
+    result
+}
+
+/// The helper's result once it has tried to start the app: when both the update step and the
+/// start fail, the error names both, the step's first.
+#[cfg_attr(not(windows), allow(dead_code))]
+fn with_restart(done: Result<(), String>, started: Result<(), String>) -> Result<(), String> {
+    match (done, started) {
+        (done, Ok(())) => done,
+        (Ok(()), Err(restart)) => Err(restart),
+        (Err(update), Err(restart)) => Err(format!("{update}; {restart}")),
+    }
 }
 
 /// Whether `exe` has `portable.txt` beside it, which makes [`Paths::for_exe`] keep its folders
@@ -2409,6 +2422,37 @@ mod tests {
         assert_eq!(std::fs::read_to_string(&exe).unwrap(), "0.1.0");
     }
 
+    /// When the update step fails and the old app can't be started again either, the error kept
+    /// for the next start names both.
+    #[cfg(windows)]
+    #[test]
+    fn a_failed_step_and_a_failed_restart_are_both_recorded() {
+        let dir = tempfile::tempdir().unwrap();
+        let paths = Paths::under(dir.path());
+        let exe = dir.path().join("artcraft-launcher.exe");
+        std::fs::write(&exe, "0.1.0").unwrap();
+        // Not a zip, so the step fails.
+        let zip = dir.path().join("artcraft-launcher-0.2.0-windows-x64-portable.zip");
+        std::fs::write(&zip, "not a zip").unwrap();
+        let step = Finish::Portable { zip, tmp: dir.path().join(PORTABLE_STAGING) };
+        let start = |_exe: &Path| Err("couldn't start artcraft-launcher.exe (denied)".to_string());
+        let exited = |_pid| true;
+        let error =
+            finish_step(&crate::suite::LAUNCHER, &system_program("cmd.exe"), &paths, 4242, exe.clone(), &step, false, &exited, &start).unwrap_err();
+        assert!(error.ends_with("; couldn't start artcraft-launcher.exe (denied)"), "{error}");
+        assert!(error.len() > "; couldn't start artcraft-launcher.exe (denied)".len(), "the step's error comes first: {error}");
+        assert_eq!(take_failure(&paths).as_deref(), Some(error.as_str()));
+    }
+
+    /// The helper's result keeps the update step's error when the start fails too.
+    #[test]
+    fn a_failed_restart_keeps_the_step_error() {
+        assert_eq!(with_restart(Ok(()), Ok(())), Ok(()));
+        assert_eq!(with_restart(Err("step".into()), Ok(())), Err("step".into()));
+        assert_eq!(with_restart(Ok(()), Err("start".into())), Err("start".into()));
+        assert_eq!(with_restart(Err("step".into()), Err("start".into())), Err("step; start".into()));
+    }
+
     /// A successful step starts the new app once.
     #[cfg(windows)]
     #[test]

crates/update/src/updater.rs

@@ -152,9 +152,10 @@ pub struct Updater {
     /// Why the last update could not be installed, until a prompt that offers the update again
     /// shows it.
     failed_reason: Option<String>,
-    /// Set when the `Updater` is dropped. An install worker sends its outcome while holding it,
-    /// and starts a finished installation's helper itself once it is set.
-    dropped: Arc<Mutex<bool>>,
+    /// Set when the app exits: the `Updater` is dropped or [`Updater::start_pending_update`] is
+    /// called. An install worker sends its outcome while holding it, and starts a finished
+    /// installation's helper itself once it is set.
+    exiting: Arc<Mutex<bool>>,
     /// The version-choice file to read in place of [`Pins::default_path`] (tests).
     pins_path: Option<PathBuf>,
     /// The launcher's choice for this app when it is not stable, as of the last check or offer.
@@ -182,7 +183,7 @@ impl Updater {
             close_wanted: false,
             paths: None,
             failed_reason: None,
-            dropped: Arc::new(Mutex::new(false)),
+            exiting: Arc::new(Mutex::new(false)),
             pins_path: None,
             managed: None,
             translate: english,
@@ -565,17 +566,17 @@ impl Updater {
     /// Call it once the app's exit is certain (a close nothing can cancel any more, in `on_exit`),
     /// whether or not the exit came through [`Action::Close`]; dropping the `Updater` calls it
     /// too. Later calls do nothing. An installation still running at the first call starts its
-    /// helper itself when it finishes, as long as the process is still running then; an app
-    /// that ends the process in `on_exit` loses an installation still in progress, since its
-    /// worker thread ends with the process. A helper that cannot start, or an installation that
-    /// failed and was not reported yet, is recorded, and the next start reports it
-    /// ([`Status::InstallFailed`]).
+    /// helper itself when it finishes, as long as the process is still running then, and no
+    /// longer reports through [`Updater::tick`]; an app that ends the process in `on_exit` loses
+    /// an installation still in progress, since its worker thread ends with the process. A
+    /// helper that cannot start, or an installation that failed and was not reported yet, is
+    /// recorded, and the next start reports it ([`Status::InstallFailed`]).
     pub fn start_pending_update(&mut self) {
         // Held while the channel is read: a worker sent its outcome before, and it is read here,
         // or it sends after and starts its helper (or records its failure) itself.
-        let dropped = Arc::clone(&self.dropped);
-        let mut dropped = dropped.lock().unwrap_or_else(PoisonError::into_inner);
-        *dropped = true;
+        let exiting = Arc::clone(&self.exiting);
+        let mut exiting = exiting.lock().unwrap_or_else(PoisonError::into_inner);
+        *exiting = true;
         // An installation that finished since the last `tick`, or whose worker ended without a word.
         // One still running is let go: it now reports through `deliver` alone.
         if let Some((rx, release)) = self.install.take()
@@ -635,11 +636,11 @@ impl Updater {
         let (tx, rx) = channel();
         let (app, version, ctx, release) = (self.app, self.version.clone(), ctx.clone(), prompt.release.clone());
         let worker_release = release.clone();
-        let dropped = handoff(&mut self.dropped);
+        let exiting = handoff(&mut self.exiting);
         let spawned = std::thread::Builder::new().name("self-update".into()).spawn(move || {
             let run = || install(app, &installed, worker_release, &version).map_err(Failure::Installer);
             let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(run)).unwrap_or_else(|_| Err(Failure::panicked()));
-            deliver(app, &tx, &dropped, paths.as_ref(), outcome);
+            deliver(app, &tx, &exiting, paths.as_ref(), outcome);
             ctx.request_repaint();
         });
         match spawned {
@@ -660,23 +661,23 @@ impl Drop for Updater {
     }
 }
 
-/// The flag a new install worker sends its outcome under (the `Updater`'s `dropped`). After
+/// The flag a new install worker sends its outcome under (the `Updater`'s `exiting`). After
 /// [`Updater::start_pending_update`] set it while the app kept running (a helper that then failed
 /// puts the update on offer again), a new flag replaces it, so that the new worker reports
 /// through `tick` instead of starting its helper at once. A worker let go before keeps the old one.
-fn handoff(dropped: &mut Arc<Mutex<bool>>) -> Arc<Mutex<bool>> {
-    if *dropped.lock().unwrap_or_else(PoisonError::into_inner) {
-        *dropped = Arc::new(Mutex::new(false));
+fn handoff(exiting: &mut Arc<Mutex<bool>>) -> Arc<Mutex<bool>> {
+    if *exiting.lock().unwrap_or_else(PoisonError::into_inner) {
+        *exiting = Arc::new(Mutex::new(false));
     }
-    Arc::clone(dropped)
+    Arc::clone(exiting)
 }
 
-/// Sends the install worker's outcome to the [`Updater`]. When the `Updater` has been dropped
-/// (the app is exiting), the worker starts a finished installation's helper itself, since
-/// nothing will start it at a close. A helper that cannot start, or a failed installation, is
-/// then recorded under `paths` for the next start.
-fn deliver(app: &App, tx: &Sender<Result<Restart, Failure>>, dropped: &Mutex<bool>, paths: Option<&Paths>, outcome: Result<Restart, Failure>) {
-    let gone = dropped.lock().unwrap_or_else(PoisonError::into_inner);
+/// Sends the install worker's outcome to the [`Updater`]. When the app is exiting (the `Updater`
+/// was dropped or [`Updater::start_pending_update`] was called), the worker starts a finished
+/// installation's helper itself, since nothing will start it at a close. A helper that cannot
+/// start, or a failed installation, is then recorded under `paths` for the next start.
+fn deliver(app: &App, tx: &Sender<Result<Restart, Failure>>, exiting: &Mutex<bool>, paths: Option<&Paths>, outcome: Result<Restart, Failure>) {
+    let gone = exiting.lock().unwrap_or_else(PoisonError::into_inner);
     let unsent = if *gone { Some(outcome) } else { tx.send(outcome).err().map(|SendError(outcome)| outcome) };
     drop(gone);
     match unsent {
@@ -1203,13 +1204,13 @@ mod tests {
     fn an_install_finishing_after_the_updater_is_dropped_starts_its_helper() {
         use std::sync::atomic::Ordering;
         let mut u = updater(Ok(None));
-        let dropped = Arc::clone(&u.dropped);
+        let exiting = Arc::clone(&u.exiting);
         let (starts, restart) = counted_restart();
         let (tx, rx) = channel();
         u.install = Some((rx, newer()));
         drop(u);
         assert_eq!(starts.load(Ordering::SeqCst), 0);
-        deliver(&crate::suite::LAUNCHER, &tx, &dropped, None, Ok(restart));
+        deliver(&crate::suite::LAUNCHER, &tx, &exiting, None, Ok(restart));
         assert_eq!(starts.load(Ordering::SeqCst), 1);
 
         // Sent while the drop has read the channel but not yet released the receiver.
@@ -1233,13 +1234,13 @@ mod tests {
         use std::sync::atomic::Ordering;
         let ctx = egui::Context::default();
         let (_dir, paths, mut u) = with_records("0.1.0");
-        let dropped = Arc::clone(&u.dropped);
+        let exiting = Arc::clone(&u.exiting);
         let (starts, restart) = counted_restart();
         let (tx, rx) = channel();
         u.install = Some((rx, newer()));
         u.start_pending_update();
         assert_eq!(starts.load(Ordering::SeqCst), 0, "nothing has finished yet");
-        deliver(&PHOTOCRAFT, &tx, &dropped, Some(&paths), Ok(restart));
+        deliver(&PHOTOCRAFT, &tx, &exiting, Some(&paths), Ok(restart));
         drop(tx);
         assert_eq!(starts.load(Ordering::SeqCst), 1, "the worker starts the helper itself");
         drop(u);
@@ -1534,11 +1535,11 @@ mod tests {
         assert_eq!(u.status(), &Status::InstallFailed("ArtCraft Launcher is still running".into()));
 
         // The retry's worker, as `start_install` starts it.
-        let dropped = handoff(&mut u.dropped);
+        let exiting = handoff(&mut u.exiting);
         let (starts, restart) = counted_restart();
         let (tx, rx) = channel();
         u.install = Some((rx, newer()));
-        deliver(&PHOTOCRAFT, &tx, &dropped, Some(&paths), Ok(restart));
+        deliver(&PHOTOCRAFT, &tx, &exiting, Some(&paths), Ok(restart));
         assert_eq!(starts.load(Ordering::SeqCst), 0, "the helper waits for the exit");
         u.tick(&ctx);
         assert!(u.installed && u.restart.is_some(), "tick picks up the retry");
@@ -1596,11 +1597,11 @@ mod tests {
         ];
         for (outcome, want) in failures {
             let (_dir, paths, mut u) = with_records("0.1.0");
-            let dropped = Arc::clone(&u.dropped);
+            let exiting = Arc::clone(&u.exiting);
             let (tx, rx) = channel();
             u.install = Some((rx, newer()));
             drop(u);
-            deliver(&PHOTOCRAFT, &tx, &dropped, Some(&paths), outcome);
+            deliver(&PHOTOCRAFT, &tx, &exiting, Some(&paths), outcome);
             let mut next = reading(&paths, "0.1.0");
             start_answered(&mut next, &ctx, Ok(None));
             assert_eq!(next.status(), &Status::InstallFailed(want.into()), "{want}");