A thread closure's destructor can panic in std::thread::spawn on Windows

[fuzzypixelz]

The following is part of the native Thread::new implementation on Windows:

// https://github.com/rust-lang/rust/blob/master/library/std/src/sys/pal/windows/thread.rs#L30
let ret = c::CreateThread(
    ptr::null_mut(),
    stack,
    Some(thread_start),
    p as *mut _,
    c::STACK_SIZE_PARAM_IS_A_RESERVATION,
    ptr::null_mut(),
);
let ret = HandleOrNull::from_raw_handle(ret);
return if let Ok(handle) = ret.try_into() {
    Ok(Thread { handle: Handle::from_inner(handle) })
} else {
    // The thread failed to start and as a result p was not consumed. Therefore, it is
    // safe to reconstruct the box so that it gets deallocated.
    drop(Box::from_raw(p));
    Err(io::Error::last_os_error())
};

If drop(Box::from_raw(p)); panics, then the error is not returned. I suggest to replace the drop statement with:

panic::catch_unwind(AssertUnwindSafe(|| drop(Box::from_raw(p))));

[fuzzypixelz]

@rustbot claim

[bjorn3]

What is exactly the problem here? I would expect a panic in the drop impl to unwind out of thread::spawn() without silently discarding the panic. A panic has a higher priority than a regular OS error IMO. A panic indicates a bug in your program, while an OS error is likely not a bug.

[fuzzypixelz]

A panic indicates a bug in your program, while an OS error is likely not a bug.

In my particular case, the OS error was caused by spawning threads in lib::atexit handlers, which was definitely a bug in the program. The fact that std::thread::spawn did not report said error made the problem needlessly hard to debug.

What I'm proposing here is catching the panic (not necessarily discarding the panic message) and returning the error. So that the caller can know about both the panic and the OS error.

[bjorn3]

What I'm proposing here is catching the panic (not necessarily discarding the panic message) and returning the error. So that the caller can know about both the panic and the OS error.

Unless you return the panic payload as the error instead of the OS error, the caller would still not be able to see the panic and resume unwinding.

[fuzzypixelz]

Unless you return the panic payload as the error instead of the OS error, the caller would still not be able to see the panic and resume unwinding.

Maybe I'm missing something specific to stdlib, but I think the panic message is preserved unless a panic hook is declared to suppress it:

fn main() {
    match std::panic::catch_unwind(|| panic!("Ahhh!")) {
        Ok(()) => unreachable!(),
        Err(_) => eprintln!("Panic avoided, backtrace preserved!"),
    };
    
}

The above prints:

thread 'main' panicked at src/main.rs:2:39:
Ahhh!
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Panic avoided, backtrace preserved!

[bjorn3]

The panic hook displays the message (if any) when using panic!(), but the panic payload is returned by catch_unwind and can be any type when using panic_any. In that case the panic hook will display Box<dyn Any> as panic message and with resume_unwind the panic hook doesn't get to run at all.

[fuzzypixelz]

The panic hook displays the message (if any) when using panic!(), but the panic payload is returned by catch_unwind and can be any type when using panic_any. In that case the panic hook will display Box<dyn Any> as panic message and with resume_unwind the panic hook doesn't get to run at all.

Thanks for the clarification, I wasn't aware there was more to it. However, catch_unwind still tells us if the destructor panicked or not. So maybe we could print a custom error message there?