Skip to content

Commit

Permalink
[HOTFIX] Resolve race condition in exit signal
Browse files Browse the repository at this point in the history
- Reinstates SIGUSR1 coordination
- Corrects signal dispatch logic
  • Loading branch information
zellio committed Nov 17, 2023
1 parent e33e3b2 commit 57f6e7a
Show file tree
Hide file tree
Showing 7 changed files with 37 additions and 46 deletions.
6 changes: 3 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "bimini"
version = "0.11.0"
version = "0.11.1"
edition = "2021"

[profile.release]
Expand Down
4 changes: 2 additions & 2 deletions bimini-core/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion bimini-core/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "bimini-core"
version = "0.11.0"
version = "0.11.1"
edition = "2021"

[dependencies]
Expand Down
2 changes: 1 addition & 1 deletion bimini-core/src/proc/child.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ impl<'a> ToEnv for Child<'a> {
(String::from("BIMINI"), String::from("true")),
(
String::from("BIMINI_VERSION"),
String::from(env!("CARGO_PKG_VERSION")),
String::from(crate::PKG_VERSION),
),
(
String::from("BIMINI_CHILD_PID"),
Expand Down
65 changes: 28 additions & 37 deletions bimini-core/src/proc/controller.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,12 @@ use nix::{
sys::{signal, wait},
unistd,
};
use std::{
process::ExitCode,
sync::{
atomic::{AtomicBool, Ordering},
Arc,
},
thread,
};
use std::{process::ExitCode, thread};

#[derive(Debug, Default)]
pub struct Controller {
signal_config: SignalConfig,
child_pid: Option<unistd::Pid>,
sf_runlock: Arc<AtomicBool>,
signal_forwarder: Option<std::thread::JoinHandle<BiminiResult<()>>>,
}

Expand Down Expand Up @@ -61,11 +53,7 @@ impl Controller {
}

#[tracing::instrument(skip_all)]
fn signal_forwarder(
sf_runlock: Arc<AtomicBool>,
signal_config: SignalConfig,
child_pid: unistd::Pid,
) -> BiminiResult<()> {
fn signal_forwarder(signal_config: SignalConfig, child_pid: unistd::Pid) -> BiminiResult<()> {
tracing::info!("Starting signal forwarding event loop");

signal::pthread_sigmask(
Expand All @@ -74,21 +62,32 @@ impl Controller {
None,
)?;

while sf_runlock.load(Ordering::Relaxed) {
match signal_config.parent_signals().wait() {
Ok(signal) if sf_runlock.load(Ordering::Relaxed) => {
tracing::info!("Passing signal to child: {signal}");
loop {
let signal = signal_config.parent_signals().wait();

signal::kill(child_pid, signal).map_err(|eno| {
if eno == errno::Errno::ESRCH {
tracing::warn!("Child was dead when forwarding signal");
}
eno
})?;
tracing::info!("Received Signal({signal:?}), dispatching");

match signal {
Ok(signal::Signal::SIGCHLD) => {
tracing::info!("Received SIGCHLD, suppressing.");
}

Ok(signal::Signal::SIGUSR1) => {
tracing::info!("Received SIGUSR1, terminating signal_forwarder loop.");
return Ok(());
}

Ok(signal) => {
tracing::trace!("Received signal {signal} after loop termination");
tracing::info!("Passing Signal({signal}) to Child(pid={child_pid})");

if let Err(errno) = signal::kill(child_pid, signal) {
match errno {
errno::Errno::ESRCH => tracing::warn!(
"Child(pid={child_pid}) was dead when forwarding, Signal({signal}) dropped."
),
errno => return Err(BiminiError::Errno(errno)),
}
}
}

Err(errno @ errno::Errno::EAGAIN | errno @ errno::Errno::EINTR) => {
Expand All @@ -101,8 +100,6 @@ impl Controller {
}
}
}

Ok(())
}

#[tracing::instrument(skip_all)]
Expand All @@ -115,14 +112,11 @@ impl Controller {
));
}

self.sf_runlock.store(true, Ordering::Release);

let runlock_clone = Arc::clone(&self.sf_runlock);
let signal_config = self.signal_config.clone();
let child_pid = self.child_pid.unwrap();

self.signal_forwarder = Some(thread::spawn(move || {
Controller::signal_forwarder(runlock_clone, signal_config, child_pid)
Controller::signal_forwarder(signal_config, child_pid)
}));

Ok(self)
Expand All @@ -146,7 +140,7 @@ impl Controller {
match wait::waitpid(any_proc, None) {
Ok(wait::WaitStatus::Exited(pid, status)) if pid == child_pid => {
tracing::info!(
"Controller child process {pid} exited normally with status {status}."
"Controller child process Child(pid={pid}) exited normally with status {status}."
);
child_exitcode = status;
}
Expand Down Expand Up @@ -214,11 +208,8 @@ impl Controller {
if rc != -1 {
tracing::trace!("Received child status code: {rc}. Cleaning up");
if let Some(signal_forwarder) = self.signal_forwarder {
tracing::trace!("Releasing signal_forwarding loop runlock");
self.sf_runlock.store(false, Ordering::Release);

tracing::trace!("Sending final SIGTERM to signal_forwarding thread");
signal::kill(unistd::Pid::from_raw(0), signal::SIGTERM)?;
tracing::trace!("Sending termination signal to signal_forwarding thread");
signal::kill(unistd::getpid(), signal::SIGUSR1)?;

tracing::trace!("Joining signal_forwarding thread");
signal_forwarder.join()??;
Expand Down
2 changes: 1 addition & 1 deletion bimini-macros/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "bimini-macros"
version = "0.11.0"
version = "0.11.1"
edition = "2021"

[lib]
Expand Down

0 comments on commit 57f6e7a

Please sign in to comment.