Detegr/rust-ctrlc

Should call user_handler() in interrupt handler?

Closed this issue · 2 comments

rom1v commented

Looking at the source code, the user handler is called from a separate thread, not from the OS interrupt handler.

As a consequence, setting a flag in the user handler and reading it from elsewhere is inherently racy.

For instance, this cannot be written without race conditions:

static INTERRUPTED_BY_USER: AtomicBool = ATOMIC_BOOL_INIT;

fn main() {
    ctrlc::set_handler(move || {
        println!("Ctrl+C detected");
        INTERRUPTED_BY_USER.store(true, Ordering::Release));
    }
    // …
    if let Err(err) = some_io_call() {
        if err.kind() == io::ErrorKind::Interrupted {
            // problem: at this point, INTERRUPTED_BY_USER may not be set by the ctrlc handler yet
            if INTERRUPTED_BY_USER.load(Ordering::Acquire) {
                println!("User requested interruption");
                quit();
            } else {
                println!("Interrupted by some other signal, retry...");
                retry();
            }
        }   
    }
}

Do you see any problems with calling the user handler from the OS interrupt handler?

EDIT: probably related to #30 by the way.

rom1v commented

I found this comment:

Running our handler in its own thread is unfortunate, but we cannot safely allow arbitrary rust code to run in the os_handler() since most operations are not async signal safe.

What if ctrlc exposed an unsafe API to register a user handler to be run directly in the os_handler()?

That comment is correct, most rust code would be unsafe when called from the OS signal handler on a POSIX system. Signal handlers are special, you can only use special async-signal safe functions, this means no memory allocation, no box or vecs, no panicing and more. Allowing registering of raw OS handlers wouldn’t be very useful, if you absolutely need to do this, you can do the libc call to setup the signal handler yourself. On windows, the OS spins up a new thread for the incoming signals, so here it would actually be safe call arbitrary rust code, but exposing this would not be cross platform.

The safe and cross platfrom solution is to provide an API with an custom safe abstraction over these atomic flag/counter type signal handlers, this can be implement without the extra thread, which means no race. Here is my suggestion #27 for adding this, but I have not had the time implement anything yet.