diff --git a/crates/update/src/process.rs b/crates/update/src/process.rs index 5aa6d42e..d76811f9 100644 --- a/crates/update/src/process.rs +++ b/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(pipe: Option) -> 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> = 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) -> 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)] diff --git a/crates/update/src/self_update.rs b/crates/update/src/self_update.rs index 40b157dc..66bfbae4 100644 --- a/crates/update/src/self_update.rs +++ b/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] diff --git a/crates/update/src/updater.rs b/crates/update/src/updater.rs index 196c862e..c45da32c 100644 --- a/crates/update/src/updater.rs +++ b/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, - /// 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>, + /// 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>, /// The version-choice file to read in place of [`Pins::default_path`] (tests). pins_path: Option, /// 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>) -> Arc> { - if *dropped.lock().unwrap_or_else(PoisonError::into_inner) { - *dropped = Arc::new(Mutex::new(false)); +fn handoff(exiting: &mut Arc>) -> Arc> { + 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>, dropped: &Mutex, paths: Option<&Paths>, outcome: Result) { - 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>, exiting: &Mutex, paths: Option<&Paths>, outcome: Result) { + 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}");