Detegr/rust-ctrlc

Async-signal safety, race condition, spurious wakeups

Closed this issue · 3 comments

  1. StaticCondvar on *nix system is implemented using pthread condition
    variables. This is problematic because pthread_cond_signal is not safe to use
    with asynchronous signal handlers:
    (http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_cond_broadcast.html)

  2. There is inherent race condition between setting DONE variable and condition
    variable notification. Consider following execution order:

    A: if !DONE.load(Ordering::Relaxed) { 
    B: super::features::DONE.store(true, Ordering::Relaxed);
    B: super::features::CVAR.notify_all();
    A: let _ = CVAR.wait(MUTEX.lock().unwrap());
    

    In this situation thread A will not notice that DONE is true, and will have
    to wait for another notification (or spurious wakeup).

  3. In case of spurious wakeup from condition variable, user handler is called
    unconditionally. This could be potentially fixed using swap / compare_and_swap.

Unfortunately that seems to be the case. I tried to avoid spinlooping by using condition variables but did not read the docs carefully enough.
Do you have any suggestions how to fix this? I have one solution which basically gets rid of all the condition variables and uses just the AtomicBool instead. This is of course the simplest and most correct one, but the spinloop requirement is a bit nasty. The sleep time could (and probably should) of course be user configurable, which would make it a bit better.

One option is what you already suggested. It is quite nice and simple, with
exception of undesirable active polling to check if signal have been delivered.

Another option is to block SIGINT delivery using signal mask. While blocked,
wait or check if delivery is pending using one of following: sigwait,
sigwaitinfo, or signalfd.

To work correctly, all threads should block SIGINT or otherwise the default
handler will be invoked in one of threads that does not block it. This is
probably not an issue if the first thing you do before creating additional
threads is to block SIGINT. The other thing is that signal mask will be
inherited by child processes. Though, I am not sure if this would be
problematic.

Thank you for the input. I think I will go with the spinloop version. I've meant this library to be just a fast and easy way to install a handler for SIGINT, and I think it's a bit beyond the scope of this library to worry about setting up signal masks etc.
The spinloop approach works and is the same in Windows too while the signal handling stuff is specific to Unix-like systems. The need for polling is unfortunate but I guess it's not that big of a deal after all: I would assume that if it is a problem for a project, the project wouldn't want to use this kind of ease-of-use wrappers anyway.