stdio handles are not UnwindSafe

[abonander]

io::{Stdout, Stderr, StdoutLock, StderrLock} are not UnwindSafe:

use std::io::{Stdout, Stderr, StdoutLock, StderrLock};
use std::panic::UnwindSafe;

fn assert_unwind_safe<T: UnwindSafe>() {}

fn main() {
    assert_unwind_safe::<Stdout>();
    assert_unwind_safe::<Stderr>();
    assert_unwind_safe::<StdoutLock<'static>>();
    assert_unwind_safe::<StderrLock<'static>>();
}

However, because they are protected by mutexes, it doesn't make sense why these handles shouldn't be UnwindSafe. It seems to be an oversight because they use ReentrantMutex internally, which transitively opts-out of UnwindSafe because it contains an UnsafeCell.

This makes io::Stdout a pain to use with slog which requires Send + Sync + UnwindSafe on its output, necessitating a second mutex wrapping the handle or a custom wrapper implementing UnwindSafe. AssertUnwindSafe is not a solution here because it doesn't implement Write for its contained type.

Either ReentrantMutex should manually implement UnwindSafe as well, or the stdio handles should. I also recommend adding delegated impls of Read, Write and BufRead to AssertUnwindSafe.

Addendum: I'm still having problems with slog but for another reason; slog_json::Json is also not UnwindSafe or Sync because it uses RefCell internally.

Addendum 2: because it uses Mutex and not ReentrantMutex, Stdin is already UnwindSafe + RefUnwindSafe, and thus StdinLock is too. My original code example didn't error for those types but I didn't notice.

[abonander]

This is a good issue for mentoring, actually. I'm willing to mentor, or otherwise resolve it myself, once someone from libs decides on a solution.

[abonander]

cc @alexcrichton @sfackler

[sfackler]

Sure, these should be unwindsafe.

[abonander]

@sfackler what is your preferred solution? Implement UnwindSafe for ReentrantMutex, or the stdio handles themsleves? And what about forwarding Read/Write/BufRead for AssertUnwindSafe?

[sfackler]

I don't feel strongly on the implementation approach. Less sure on forwarding impls for AssertUnwindSafe. We do currently forward FnOnce but that's just to make catch_unwind(AssertUnwindSafe(|| ...)) work. Not sure we want to open the can of worms of forwarding impls for every trait.

[alexcrichton]

Could this perhaps be solved with an unsafe implementation of UnwindSafe/RefUnwindSafe for ReentrantMutex?

[KamilaBorowska]

I think UnwindSafe makes sense for ReentrantMutex, as it can be poisoned, just like Mutex.

[abonander]

@kennytm Can you mark this Mentor + Easy?

Mentoring Instructions

This one is really easy. std::panic::catch_unwind() uses a marker trait, UnwindSafe and enforces it on the closure passed to it. This trait is mostly a speedbump to force users to think about panic-safety if they try to catch a panic. It's implemented for everything aside from anything containing UnsafeCell or &mut; if a type uses either one of these but wants to be UnwindSafe, it has to manually opt-in by implementing it, just like with Send or Sync. However, it is not unsafe to implement UnwindSafe.

The stdio handles (e.g. io::Stdout) should be UnwindSafe because they use a mutex internally for thread-safety; since threads allow shared data to survive a panic and signal that via poisoning, it stands to reason that anything that is thread-safe should also be unwind-safe as well (though there isn't currently a blanket impl of UnwindSafe for T: Sync).

However, the stdio handles use an internal-only mutex type that is not std::sync::Mutex (which manually implements UnwindSafe since it contains UnsafeCell). Instead they use sys_common::ReentrantMutex (a reentrant mutex can be locked multiple times by the same thread; not generally useful in Rust's ownership scheme but the stdio handles use it).

While sync::Mutex manually implements UnwindSafe and RefUnwindSafe, it appears this was accidentally omitted on sys_common::ReentrantMutex. However, it is still a candidate for both traits because it implements the same poisoning scheme as Mutex.

The solution is simple, add impl<T> UnwindSafe for ReentrantMutex<T> and impl<T> RefUnwindSafe for ReentrantMutex<T> to this file, probably just after the linked line: https://github.com/rust-lang/rust/blob/master/src/libstd/sys_common/remutex.rs#L29

I would also like to see regression tests ensuring that Stdout, StdoutLock, Stderr and StderrLock remain UnwindSafe and RefUnwindSafe. This is simple to do using a function like this:

fn assert_unwind_safe<T: UnwindSafe + RefUnwindSafe>() {}

And call it using the turbofish operator and the type to test, e.g. assert_unwind_safe::<StdoutLock<'static>>(). You can stick all that in this submod: https://github.com/rust-lang/rust/blob/master/src/libstd/io/stdio.rs#L714

(Stdin and StdinLock use the regular Mutex because they don't need reentrancy and thus are actually fine already; it's necessary to avoid deadlocks with the output handles because they have to lock before calling .write_fmt(), so any prints inside the formatting traits could deadlock if the lock wasn't reentrant).