From 4f1b58f20c35295ae5a3696fc91f796490af97be Mon Sep 17 00:00:00 2001 From: Tim Besard Date: Mon, 30 Jan 2023 15:08:49 +0100 Subject: [PATCH 1/5] launcher: replace parent process on supported platforms. This simplifies use of juliaup with, e.g., debuggers. Database updates are executed in a forked process. --- Cargo.toml | 2 +- src/bin/julialauncher.rs | 89 ++++++++++++++++++++++++---------------- 2 files changed, 54 insertions(+), 37 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 59625432..886e4f50 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -69,7 +69,7 @@ reqwest = { version = "0.11.18", default-features = false, features = ["blocking reqwest = { version = "0.11.18", default-features = false, features = ["blocking", "rustls-tls-native-roots", "socks"] } [target.'cfg(not(windows))'.dependencies] -nix = "0.27.1" +nix = { version = "0.27.1", features = ["process"] } [build-dependencies] anyhow = "1.0.72" diff --git a/src/bin/julialauncher.rs b/src/bin/julialauncher.rs index 1201237f..4dcda254 100644 --- a/src/bin/julialauncher.rs +++ b/src/bin/julialauncher.rs @@ -8,6 +8,12 @@ use juliaup::versions_file::load_versions_db; use normpath::PathExt; use std::path::Path; use std::path::PathBuf; +#[cfg(windows)] +use std::process::Command; +#[cfg(not(windows))] +use std::os::unix::process::CommandExt; +#[cfg(not(windows))] +use nix::unistd::{fork, ForkResult}; #[derive(thiserror::Error, Debug)] #[error("{msg}")] @@ -296,53 +302,64 @@ fn run_app() -> Result { // process to handle things. ctrlc::set_handler(|| ()).with_context(|| "Failed to set the Ctrl-C handler.")?; - let mut child_process = std::process::Command::new(julia_path) - .args(&new_args) - .spawn() - .with_context(|| "The Julia launcher failed to start Julia.")?; // TODO Maybe include the command we actually tried to start? - - run_versiondb_update(&config_file).with_context(|| "Failed to run version db update")?; - - run_selfupdate(&config_file).with_context(|| "Failed to run selfupdate.")?; + // On *nix platforms we replace the current process with the Julia one. + // This simplifies use in e.g. debuggers, but requires that we fork off + // a subprocess to do the selfupdate and versiondb update. + #[cfg(not(windows))] + match unsafe{fork()} { + Ok(ForkResult::Parent { child: _, .. }) => { + // XXX: because we never wait on our child, it'll appear as a zombie + // process until the parent exits. it's also not possible to disable + // SIGCHLD handling, because that breaks Julia. + + std::process::Command::new(julia_path) + .args(&new_args) + .exec(); + + // this never executes + Ok(0) + } + Ok(ForkResult::Child) => { + // NOTE: It is unsafe to perform async-signal-unsafe operations from + // multithreaded programs, so for complex functionality like + // selfupdate to work julialauncher needs to remain single-threaded. + // Ref: https://docs.rs/nix/latest/nix/unistd/fn.fork.html#safety - let status = child_process - .wait() - .with_context(|| "Failed to wait for Julia process to finish.")?; + run_versiondb_update(&config_file).with_context(|| "Failed to run version db update")?; - let code = match status.code() { - Some(code) => code, - None => { - #[cfg(not(windows))] - { - use std::os::unix::process::ExitStatusExt; + run_selfupdate(&config_file).with_context(|| "Failed to run selfupdate.")?; - let signal = status.signal(); + // this exitcode won't be seen by anybody + Ok(0) + } + Err(_) => panic!("Could not fork!"), + } - if let Some(signal) = signal { - let signal = nix::sys::signal::Signal::try_from(signal) - .with_context(|| format!("Unknown signal value {}.", signal))?; + // On other platforms (i.e., Windows) we just spawn a subprocess + #[cfg(windows)] + { + let mut child_process = std::process::Command::new(julia_path) + .args(&new_args) + .spawn() + .with_context(|| "The Julia launcher failed to start Julia.")?; // TODO Maybe include the command we actually tried to start? - nix::sys::signal::raise(signal).with_context(|| "Failed to raise signal.")?; + run_versiondb_update(&config_file).with_context(|| "Failed to run version db update")?; - // We raise the signal twice because SIGSEGV and SIGBUS require that - // https://github.com/JuliaLang/juliaup/pull/525#issuecomment-1353526900 - // https://github.com/rust-lang/rust/blob/984eab57f708e62c09b3d708033fe620130b5f39/library/std/src/sys/unix/stack_overflow.rs#L60-L80 - nix::sys::signal::raise(signal).with_context(|| "Failed to raise signal.")?; + run_selfupdate(&config_file).with_context(|| "Failed to run selfupdate.")?; - anyhow::bail!("Maybe we should never reach this?"); - } else { - anyhow::bail!("We weren't able to extract a signal, this should never happen."); - } - } + let status = child_process + .wait() + .with_context(|| "Failed to wait for Julia process to finish.")?; - #[cfg(windows)] - { + let code = match status.code() { + Some(code) => code, + None => { anyhow::bail!("There is no exit code, that should not be possible on Windows."); } - } - }; + }; - Ok(code) + Ok(code) + } } fn main() -> Result { From 75bd65ded2a75c9a337b33c1755fe42a897dace6 Mon Sep 17 00:00:00 2001 From: Tim Besard Date: Tue, 31 Jan 2023 09:40:33 +0100 Subject: [PATCH 2/5] Use double fork to prevent zombies. --- src/bin/julialauncher.rs | 62 +++++++++++++++++++++++++++++----------- 1 file changed, 45 insertions(+), 17 deletions(-) diff --git a/src/bin/julialauncher.rs b/src/bin/julialauncher.rs index 4dcda254..75304a58 100644 --- a/src/bin/julialauncher.rs +++ b/src/bin/julialauncher.rs @@ -13,7 +13,7 @@ use std::process::Command; #[cfg(not(windows))] use std::os::unix::process::CommandExt; #[cfg(not(windows))] -use nix::unistd::{fork, ForkResult}; +use nix::{sys::wait::{waitpid, WaitStatus}, unistd::{fork, ForkResult}}; #[derive(thiserror::Error, Debug)] #[error("{msg}")] @@ -307,32 +307,59 @@ fn run_app() -> Result { // a subprocess to do the selfupdate and versiondb update. #[cfg(not(windows))] match unsafe{fork()} { - Ok(ForkResult::Parent { child: _, .. }) => { - // XXX: because we never wait on our child, it'll appear as a zombie - // process until the parent exits. it's also not possible to disable - // SIGCHLD handling, because that breaks Julia. + // NOTE: It is unsafe to perform async-signal-unsafe operations from + // forked multithreaded programs, so for complex functionality like + // selfupdate to work julialauncher needs to remain single-threaded. + // Ref: https://docs.rs/nix/latest/nix/unistd/fn.fork.html#safety + + Ok(ForkResult::Parent { child, .. }) => { + // wait for the daemon-spawning child to finish + match waitpid(child, None) { + Ok(WaitStatus::Exited(_, code)) => { + if code != 0 { + panic!("Could not fork (child process exited with code: {})", code) + } + } + Ok(_) => { + panic!("Could not fork (child process did not exit normally)"); + } + Err(e) => { + panic!("Could not fork (error waiting for child process, {})", e); + } + } + // replace the current process std::process::Command::new(julia_path) .args(&new_args) .exec(); - // this never executes + // this is never reached Ok(0) } Ok(ForkResult::Child) => { - // NOTE: It is unsafe to perform async-signal-unsafe operations from - // multithreaded programs, so for complex functionality like - // selfupdate to work julialauncher needs to remain single-threaded. - // Ref: https://docs.rs/nix/latest/nix/unistd/fn.fork.html#safety - - run_versiondb_update(&config_file).with_context(|| "Failed to run version db update")?; - - run_selfupdate(&config_file).with_context(|| "Failed to run selfupdate.")?; + // double-fork to prevent zombies + match unsafe{fork()} { + Ok(ForkResult::Parent { child: _, .. }) => { + // we don't do anything here so that this process can be + // reaped immediately + } + Ok(ForkResult::Child) => { + // this is where we perform the actual work. we don't do + // any typical daemon-y things (like detaching the TTY) + // so that any error output is still visible. + + run_versiondb_update(&config_file) + .with_context(|| "Failed to run version db update")?; + + run_selfupdate(&config_file) + .with_context(|| "Failed to run selfupdate.")?; + } + Err(_) => panic!("Could not double-fork"), + } - // this exitcode won't be seen by anybody Ok(0) } - Err(_) => panic!("Could not fork!"), + Err(_) => panic!("Could not fork"), } // On other platforms (i.e., Windows) we just spawn a subprocess @@ -343,7 +370,8 @@ fn run_app() -> Result { .spawn() .with_context(|| "The Julia launcher failed to start Julia.")?; // TODO Maybe include the command we actually tried to start? - run_versiondb_update(&config_file).with_context(|| "Failed to run version db update")?; + run_versiondb_update(&config_file) + .with_context(|| "Failed to run version db update")?; run_selfupdate(&config_file).with_context(|| "Failed to run selfupdate.")?; From 7f674a26cf96770cbb0080aa8eba964146c7c5f8 Mon Sep 17 00:00:00 2001 From: Tim Besard Date: Thu, 4 Jan 2024 10:48:16 +0100 Subject: [PATCH 3/5] Format code. --- src/bin/julialauncher.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/bin/julialauncher.rs b/src/bin/julialauncher.rs index 75304a58..3712b156 100644 --- a/src/bin/julialauncher.rs +++ b/src/bin/julialauncher.rs @@ -5,15 +5,18 @@ use juliaup::config_file::{load_config_db, JuliaupConfig, JuliaupConfigChannel}; use juliaup::global_paths::get_paths; use juliaup::jsonstructs_versionsdb::JuliaupVersionDB; use juliaup::versions_file::load_versions_db; +#[cfg(not(windows))] +use nix::{ + sys::wait::{waitpid, WaitStatus}, + unistd::{fork, ForkResult}, +}; use normpath::PathExt; +#[cfg(not(windows))] +use std::os::unix::process::CommandExt; use std::path::Path; use std::path::PathBuf; #[cfg(windows)] use std::process::Command; -#[cfg(not(windows))] -use std::os::unix::process::CommandExt; -#[cfg(not(windows))] -use nix::{sys::wait::{waitpid, WaitStatus}, unistd::{fork, ForkResult}}; #[derive(thiserror::Error, Debug)] #[error("{msg}")] @@ -306,12 +309,11 @@ fn run_app() -> Result { // This simplifies use in e.g. debuggers, but requires that we fork off // a subprocess to do the selfupdate and versiondb update. #[cfg(not(windows))] - match unsafe{fork()} { + match unsafe { fork() } { // NOTE: It is unsafe to perform async-signal-unsafe operations from // forked multithreaded programs, so for complex functionality like // selfupdate to work julialauncher needs to remain single-threaded. // Ref: https://docs.rs/nix/latest/nix/unistd/fn.fork.html#safety - Ok(ForkResult::Parent { child, .. }) => { // wait for the daemon-spawning child to finish match waitpid(child, None) { @@ -338,7 +340,7 @@ fn run_app() -> Result { } Ok(ForkResult::Child) => { // double-fork to prevent zombies - match unsafe{fork()} { + match unsafe { fork() } { Ok(ForkResult::Parent { child: _, .. }) => { // we don't do anything here so that this process can be // reaped immediately @@ -351,8 +353,7 @@ fn run_app() -> Result { run_versiondb_update(&config_file) .with_context(|| "Failed to run version db update")?; - run_selfupdate(&config_file) - .with_context(|| "Failed to run selfupdate.")?; + run_selfupdate(&config_file).with_context(|| "Failed to run selfupdate.")?; } Err(_) => panic!("Could not double-fork"), } @@ -370,8 +371,7 @@ fn run_app() -> Result { .spawn() .with_context(|| "The Julia launcher failed to start Julia.")?; // TODO Maybe include the command we actually tried to start? - run_versiondb_update(&config_file) - .with_context(|| "Failed to run version db update")?; + run_versiondb_update(&config_file).with_context(|| "Failed to run version db update")?; run_selfupdate(&config_file).with_context(|| "Failed to run selfupdate.")?; From f873b8fe9563d42de095dd30be58c0a3d69ea8dd Mon Sep 17 00:00:00 2001 From: Tim Besard Date: Thu, 4 Jan 2024 10:58:11 +0100 Subject: [PATCH 4/5] Only register the CTRL-C handler when it's safe to do so. --- src/bin/julialauncher.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/bin/julialauncher.rs b/src/bin/julialauncher.rs index 3712b156..31784d68 100644 --- a/src/bin/julialauncher.rs +++ b/src/bin/julialauncher.rs @@ -301,10 +301,6 @@ fn run_app() -> Result { } } - // We set a Ctrl-C handler here that just doesn't do anything, as we want the Julia child - // process to handle things. - ctrlc::set_handler(|| ()).with_context(|| "Failed to set the Ctrl-C handler.")?; - // On *nix platforms we replace the current process with the Julia one. // This simplifies use in e.g. debuggers, but requires that we fork off // a subprocess to do the selfupdate and versiondb update. @@ -350,6 +346,11 @@ fn run_app() -> Result { // any typical daemon-y things (like detaching the TTY) // so that any error output is still visible. + // We set a Ctrl-C handler here that just doesn't do anything, as we want the Julia child + // process to handle things. + ctrlc::set_handler(|| ()) + .with_context(|| "Failed to set the Ctrl-C handler.")?; + run_versiondb_update(&config_file) .with_context(|| "Failed to run version db update")?; @@ -366,6 +367,10 @@ fn run_app() -> Result { // On other platforms (i.e., Windows) we just spawn a subprocess #[cfg(windows)] { + // We set a Ctrl-C handler here that just doesn't do anything, as we want the Julia child + // process to handle things. + ctrlc::set_handler(|| ()).with_context(|| "Failed to set the Ctrl-C handler.")?; + let mut child_process = std::process::Command::new(julia_path) .args(&new_args) .spawn() From 4eb8034ebfe7111fed2aab99cb92b1fdfc1d5717 Mon Sep 17 00:00:00 2001 From: Tim Besard Date: Thu, 4 Jan 2024 11:01:47 +0100 Subject: [PATCH 5/5] Try removing an unused import. --- src/bin/julialauncher.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/bin/julialauncher.rs b/src/bin/julialauncher.rs index 31784d68..3e9a0cfc 100644 --- a/src/bin/julialauncher.rs +++ b/src/bin/julialauncher.rs @@ -15,8 +15,6 @@ use normpath::PathExt; use std::os::unix::process::CommandExt; use std::path::Path; use std::path::PathBuf; -#[cfg(windows)] -use std::process::Command; #[derive(thiserror::Error, Debug)] #[error("{msg}")]