Skip to content

Allow FnMut handler functions and placate clippy #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 15, 2021
Merged

Allow FnMut handler functions and placate clippy #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,4 @@ name = "simple_signal"

[dependencies]
libc = "0.2"

[dependencies.lazy_static]
version = "1.0"
optional = true

[features]
default = ["stable"]
nightly = []
stable = ["lazy_static"]
lazy_static = "1.4"
3 changes: 0 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,3 @@ fn main() {

#### Try the example yourself
`cargo run --example readme_example`

## Building
If you're using a nightly compiler, I suggest building with `cargo build --features nightly` to avoid the dependency on *lazy_static*. On stable and beta compilers, just run `cargo build`.
63 changes: 22 additions & 41 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,6 @@
//! A simple wrapper for handling Unix process signals.

#![cfg_attr(feature="nightly", feature(static_condvar))]
#![cfg_attr(feature="nightly", feature(static_mutex))]

#[cfg(feature="stable")]
#[macro_use]
extern crate lazy_static;

use std::sync::atomic::Ordering;
use std::thread;

#[derive(Clone, Copy, PartialEq, Eq, Debug)]
pub enum Signal {
Expand All @@ -25,35 +17,20 @@ pub enum Signal {
Term,
}

#[cfg(feature="nightly")]
mod features {
use std::sync::atomic::{AtomicUsize, ATOMIC_USIZE_INIT};
use std::sync::{StaticCondvar, CONDVAR_INIT, StaticMutex, MUTEX_INIT};
pub static CVAR: StaticCondvar = CONDVAR_INIT;
pub static MUTEX: StaticMutex = MUTEX_INIT;
pub static MASK: AtomicUsize = ATOMIC_USIZE_INIT;
use std::sync::atomic::AtomicUsize;
use std::sync::{Condvar, Mutex};
lazy_static::lazy_static! {
pub static ref CVAR: Condvar = Condvar::new();
pub static ref MUTEX: Mutex<()> = Mutex::new(());
}

#[cfg(not(feature="nightly"))]
mod features {
use std::sync::atomic::{AtomicUsize, ATOMIC_USIZE_INIT};
use std::sync::{Condvar, Mutex};
lazy_static! {
pub static ref CVAR: Condvar = Condvar::new();
pub static ref MUTEX: Mutex<bool> = Mutex::new(false);
}
pub static MASK: AtomicUsize = ATOMIC_USIZE_INIT;
}

use self::features::*;
pub static MASK: AtomicUsize = AtomicUsize::new(0);

#[cfg(unix)]
mod platform {
extern crate libc;

use self::libc::{c_int, signal, sighandler_t};
use self::libc::{SIGHUP, SIGINT, SIGQUIT, SIGILL, SIGABRT, SIGFPE, SIGKILL, SIGSEGV, SIGPIPE, SIGALRM, SIGTERM};
use std::mem;
use std::sync::atomic::Ordering;
use super::Signal;

Expand All @@ -74,13 +51,13 @@ mod platform {
};

loop {
let prev_mask = super::features::MASK.load(Ordering::Relaxed);
let prev_mask = super::MASK.load(Ordering::Relaxed);
let new_mask = prev_mask | mask;
if super::features::MASK.compare_and_swap(prev_mask, new_mask, Ordering::Relaxed) == new_mask {
if super::MASK.compare_exchange(prev_mask, new_mask, Ordering::Relaxed, Ordering::Relaxed).is_ok() {
break;
}
}
super::features::CVAR.notify_all();
super::CVAR.notify_all();
}

#[inline]
Expand All @@ -99,7 +76,7 @@ mod platform {
Signal::Term => SIGTERM,
};

signal(os_sig, mem::transmute::<_, sighandler_t>(handler as extern "C" fn(_)));
signal(os_sig, handler as extern "C" fn(_) as sighandler_t);
}
}

Expand All @@ -112,17 +89,17 @@ use self::platform::*;
/// use simple_signal::{self, Signal};
/// simple_signal::set_handler(&[Signal::Int, Signal::Term], |signals| println!("Caught: {:?}", signals));
/// ```
pub fn set_handler<F>(signals: &[Signal], user_handler: F) where F: Fn(&[Signal]) + 'static + Send {
pub fn set_handler<F>(signals: &[Signal], mut user_handler: F) where F: FnMut(&[Signal]) + 'static + Send {
for &signal in signals.iter() {
unsafe { set_os_handler(signal) }
}
thread::spawn(move || {
std::thread::spawn(move || {
let mut signals = Vec::new();
let mut guard = MUTEX.lock().unwrap();
loop {
let mask = MASK.load(Ordering::Relaxed);
let mask = MASK.swap(0, Ordering::Relaxed);
if mask == 0 {
let _ = CVAR.wait(MUTEX.lock().unwrap());
thread::yield_now();
guard = CVAR.wait(guard).unwrap();
continue;
}
signals.clear();
Expand All @@ -137,7 +114,6 @@ pub fn set_handler<F>(signals: &[Signal], user_handler: F) where F: Fn(&[Signal]
if mask & 256 != 0 { signals.push(Signal::Pipe) }
if mask & 512 != 0 { signals.push(Signal::Alrm) }
if mask & 1024 != 0 { signals.push(Signal::Term) }
MASK.store(0, Ordering::Relaxed);
user_handler(&signals);
}
});
Expand All @@ -150,7 +126,7 @@ mod test {
use std::sync::mpsc::sync_channel;
use self::libc::c_int;
use self::libc::{SIGHUP, SIGINT, SIGQUIT, SIGILL, SIGABRT, SIGFPE, SIGKILL, SIGSEGV, SIGPIPE, SIGALRM, SIGTERM};
use super::{Signal};
use super::Signal;
use super::platform::handler;

fn to_os_signal(signal: Signal) -> c_int {
Expand All @@ -173,7 +149,12 @@ mod test {
fn all_signals() {
let signals = [Signal::Hup, Signal::Int, Signal::Quit, Signal::Abrt, Signal::Term];
let (tx, rx) = sync_channel(0);
super::set_handler(&signals, move |signals| tx.send(signals.to_owned()).unwrap());
let mut signal_count = 0;
super::set_handler(&signals, move |signals| {
signal_count += signals.len();
println!("Handled {} signals", signal_count);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Is this "println!" here for a reason or it is just kind of debugging forgotten?

Copy link
Author

@bossmc bossmc Apr 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's there for a reason, but not a great one 😄 I wanted to prove the handler could be FnMut by testing with one that mutates captured variables, so I added the signal_count variable - then rust complained that it was unreferenced (since the code only increments it).

I added the printf to "use" the variable (and you can see the output with cargo test -- --nocapture) - I considered sending the current value out of the closure on a new channel and asserting the current count on the outside (as the test already does for the signals), but that's not really testing anything meaningful.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, okay, I got it :)

I doubt that the handler could really be FnMut, I have a strong feeling that we cannot directly mutate anything from another (detached) thread via &mut reference.

For example consider the main thread has exited before the one in set_handler, then any access from it will trigger segmentation fault.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FnMut allows mutation of captured variables, which is independent to accessing &mut references. The use case I wanted was something like:

async fn main() {
  let (rx, tx) = tokio::sync::oneshot();
  let mut tx = Some(tx);
  set_handler(&signals, move |_| {
    if let Some(tx) = tx.take() {        // `take` needs `FnMut`
      tx.send("shutdown now please");
    }
  })

  tokio::spawn(...); // etc.

  rx.await;

  info!("Shutting down now");
}

As the handler is 'static it can't capture references (mutable or otherwise) which prevents the segfault you were thinking about.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay that sounds perfectly reasonable!

tx.send(signals.to_owned()).unwrap();
});
// Check all signals one-by-one.
for &signal in signals.iter() {
handler(to_os_signal(signal));
Expand Down