JoinGuard::join returning an Err is **really** unsafe

[tomaka]

Exception safety is a very well-known problem in C++.

Consider this:

struct Foo {
    int* ptr;
};

void modify(Foo& value) {
    delete foo.ptr;
    foo.ptr = new int;
}

void error_proof_modify(Foo& value) {
    try {
        modify(value);
    } catch(...) {
        cerr << "Error, but let's continue!";
    }
}

If the call to new throws an exception, the error gets caught and the execution continue. However the Foo object is now in an invalid state with its pointer pointing to memory that has been free'd.

Before #20615 and before the change in Send this is not possible in Rust, because when a thread panics all objects that are local to this thread are no longer accessible. It's also the reason why mutexes are poisoned in case of a panic.

But now that it's possible to send local variables to other threads, and ignore when this other thread panics, situations like this can totally happen.

Let's take this code for example:

pub struct Foo {
    val1: int,
    val2: int,
    val3: int,
    calculation_result: int,    // must always be equal to val1+val2+val3
}

impl Foo {
    pub fn set_val1(&mut self, value: int) {
        self.val1 = value;
        self.update_calculation();
    }
    pub fn set_val2(&mut self, value: int) {
        self.val2 = value;
        self.update_calculation();
    }
    pub fn set_val3(&mut self, value: int) {
        self.val3 = value;
        self.update_calculation();
    }

    fn update_calculation(&mut self) {
        if self.val1 == 127 { panic!("for the sake of this example, we panic") };
        self.calculation_result = self.val1 + self.val2 + self.val3;
    }
}

Looks safe, right?

But what if you use that "ignore panics" trick?

let mut foo: Foo = ...;  // whatever

Thread::scoped(|| {
    foo.set_val1(127);
}).join();   // ignoring the panic

use_foo(&foo);   // continue using `foo`

At the end of this code, you have a Foo object in an invalid state because calculation_result is not equal to val1+val2+val3 as it should be. Continuing to use the Foo object in this invalid state could lead to weird and hard to debug results (depending on the rest of the code).

With the latest reforms in thread, library writer really have to take exception safety (or "panic safety") into account by avoiding the kind of code written above. You may argue that it's just the fault of the person who wrote the Foo struct. The problem is that exception safety is hard. Too hard to get right.

In my opinion, JoinGuard::join should just panic if the underlying thread panicked, without leaving the choice to the user. The only situation in which a try_join function (or another name) is safe is for JoinGuard<'static>.

cc @aturon

[sfackler]

This is already something you have to worry about, since destructors would be able to look at foo during unwinding in the thread as well.

[jfager]

These examples are different classes of bugs: the C++ code shows a critical memory safety issue, while the Rust code shows a logic error that doesn't fall within the realm of 'safety' in the sense that Rust means.

How is the rust example so different from the obviously "just a bug" behavior of:

fn update_calculation(&mut self) {
    if self.val1 == 127 { return; };
    self.calculation_result = self.val1 + self.val2 + self.val3;
}

?

[pythonesque]

This is a really good point that I overlooked in the API. I agree with you, tomaka.

[tomaka]

How is the rust example so different from the obviously "just a bug" behavior of:

In my example I wrote if self.val1 == 127 { panic!() }, but in real code things that can panic look like unwrap(), my_vec[i], etc., which you use when you know that you can't recover from them. That's why bringing the possibility to recover from a panic is a bad idea.

Here is a better example. This code below looks reasonable but has exception-safety related issues:

fn update_calculation(&mut self) {
    self.calculation_result = self.val1.checked_add(self.val2).unwrap().checked_add(self.val3).unwrap();
}

pub fn set_val1(&mut self, value: int) {
    self.val1 = value;
    self.update_calculation();
}

If you want to "fix" this code, you would have to write:

fn update_calculation(&mut self) -> Result<(), ()> {
    let tmp1 = match self.val1.checked_add(self.val2) {
        Some(v) => v,
        None => return Err(())
    };

    let tmp2 = match tmp1.checked_add(self.val3) {
        Some(v) => v,
        None => return Err(())
    };

    self.calculation_result = tmp2;
    Ok(())
}

pub fn set_val1(&mut self, value: int) {
    let previous_value = self.val1;
    self.val1 = value;
    if let Err(_) = self.update_calculation() {
        self.val1 = previous_value;
        panic!();
    }
}

This is why exception safety is hard. And I'd like to emphasize that this is just a trivial case.

[pythonesque]

@sfackler Sure, it's easy to replicate in a single thread:

#![feature(unsafe_destructor)]

#[derive(Show)] struct Foo { foo: u8 }

#[unsafe_destructor]
impl<'a, 'b> Drop for Foo {
    fn drop(&mut self) {
        println!("Foo: {:?}", self.foo);
    }
}

/// Invariant: *x = 2 after this is called.
fn foo<'a>(x: &'a mut u8) {
    if *x == 0 {
        panic!("oops");
    }
    *x = 2;
}

fn main() {
    let mut x = Foo {foo: 0 };
    {
        foo(&mut x.foo);
    }
}

The difference is that you only have to worry about the exception safety violations in destructors. Destructors already have to do this if you don't abort on panic. That's fundamental to unwinding. The problem here that tomaka is pointing out is that this change causes non-destructors to have to worry about it.

The other key difference with destructors is that destructors can make no guarantees about where they are called (for the most part) so they can't really rely on functions being run after the type is instantiated. In this case, the guarantee foo made is not one the destructor could assume anyway. However, straightforward regular code does need to be able to assume that function calls fulfill their promises (often encoded in the type signature). If it can't, that's where you get exception safety violations.

Personally I think making .join() panic unless you use 'static is a great idea. It is more consistent with how I view bounded lifetime threads--they should for the most part work like function calls that just happen to be synchronous. When a regular function call panics, you know for sure that all code is going to get unwound. This would encourage people to use Results for bounded lifetime thread return values, just like normal functions that can fail.

[tomaka]

Destructors are usually either really low-level or don't require the object to be in a valid state, so it's a far smaller problem.

[pythonesque]

By the way, this is really sometimes a problem even for existing threads in non-destructors, unless we abort on panic. If you use atomics, for example, which have no poison value, it's very easy to get exception safety violations by not coding transactionally. So this is also an argument for abort on panic being the default (among other things) but this is a larger discussion. I think at least for now, making non-destructors worry about exception safety in cases like this is not a good idea.

On another note, if people use tricks like this to make sure their or third party code can't panic, rather than because they don't want to deal with it, that is a good sign that we need a noexcept.

[aturon]

Sorry for taking so long to write in explicitly on this thread, though I've been mulling over the API issue for a while.

I think that @tomaka and @pythonesque are right in that in general, we make it somewhat difficult (though by no means impossible) to run into exception safety problems in "normal" (especially safe) code. You have to use RefCell, or dig out a value from PoisonError in a Mutex, in order to observe problems outside of destructors.

The current scoped API, however, risks changing this equation a bit in that "all" you have to do is throw away a Result to start observing potentially bad data:

let _ = Thread::scoped(f).join();
// read some data that f had &mut access to

The question is, is the #[must_use] attribute on Result (together with the fact that you have to at least explicitly throw it out as above) enough of a warning here? Note, we do allow you to make similar observations with mutexes, but you have to dig it out.

I'm not really sure. I feel like the Result is probably enough, personally, but we could also consider splitting up join into a panicking version and a join_static version that has today's signature, for example. I'd appreciate other API sketches if people have more ideas.

I also played with a much crazier version, but I think it's far too "clever" to be of use.

@pythonesque

If you use atomics, for example, which have no poison value, it's very easy to get exception safety violations by not coding transactionally.

This I disagree with: if you're using atomics, you're coding transactionally almost by definition: since concurrent threads can observe the state after every atomic update, each update must uphold all needed invariants. This is why lock-free data structures are so fun to write :)

[tomaka]

I have never thought of #[must_use] as a "must handle". There are several situations where you don't care whether the function call succeeded and where you can legitimately throw away the Result. For example writing a "goodbye" message just before closing a socket, or sending the result of a calculation from a background thread on a channel.

After some thoughts, for me the API should ideally look like this:

• Thread::scoped returns a ScopedJoinGuard whose join method panics.
• Thread::spawn returns a JoinGuard (or another name) whose join method returns a Result.

After all, I can see three use cases:

• You want to spawn a background thread, in which case you'd call Thread::spawn() then detach (instead of just Thread::spawn() today). For me this is the rarest case.
• You want to spawn a thread but have some kind of ownership over it, in which case you'd call Thread::spawn() (today you'd call Thread::scoped()).
• You want to spawn a thread that manipulates your local variables, in which case you'd call Thread::scoped().

I don't know if that will pass, since I think the API used to look kind of like this and since it's a bigger change than a simple modification to a safety problem.

Other important problem: the destructor of JoinGuard simply joins the thread and ignores the result. This should be fixed too. I don't know what the state of linear types is, but that would be a good situation to use them. Alternatively, in my opinion panic in the destructor unless the current thread is unwinding.

or dig out a value from PoisonError in a Mutex

Ouch, I didn't know about this one.

I'd be in favor or making into_guard unsafe. I don't see what would be a legitimate use-case of this outside of unsafe code.

I guess that this whole discussion is supposed to be in the "threads RFC" instead of an issue. Unfortunately like many people (I think) I don't have the time to read RFCs, and I spot problems once things are implemented.

[pythonesque]

@tomaka I'm okay with the poison stuff because you really have to explicitly abandon exception safety in order to use the Mutex that way. My concern is that there is nothing stopping you from just ignoring the value of the returned Thread except for #[must_use], so the path of least resistance may be giving up exception safety (especially since in most cases, Rust programmers are being correctly trained to use try! rather than unwrap()). Unfortunately I don't really see a way to make it much easier to do the right thing without real linear types (so you could actually force the Result of joining a thread to be handled explicitly) or just panicking (the bomb idea is really interesting though... I could actually be persuaded to like that, but it might be hard to get through an RFC).