Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rust does not guarantee thread-safety #26215

Closed
rightfold opened this issue Jun 11, 2015 · 12 comments
Closed

Rust does not guarantee thread-safety #26215

rightfold opened this issue Jun 11, 2015 · 12 comments

Comments

@rightfold
Copy link

According to the Rust website, Rust guarantees thread safety. However, I managed to write a program that appears to be thread-unsafe while it does not use external resources or unsafe:

use std::sync::{Arc, Mutex};
use std::thread;

fn main() {
    let p = Arc::new(Mutex::new(0));

    for _ in 1..100 {
        let x = p.clone();
        thread::spawn(move || {
            let n;
            {
                let mut x_ = x.lock().unwrap();
                *x_ += 1;
                n = *x_;
            }
            thread::sleep_ms(50);
            {
                // because Rust guarantees thread-safety, we can safely assume x hasn't been changed
                let x_ = x.lock().unwrap();
                if *x_ != n {
                    panic!();
                }
            }
        });
    }

    thread::sleep_ms(2000);
}

Expected output:
No output and exit status 0.

Actual output:
This program panics.

@llogiq
Copy link
Contributor

llogiq commented Jun 11, 2015

Could you please explain where you believe the thread-unsafety lies?

If you change the last line to

    for j in threads { let _ = j.join(); }

You can see the real reason for the panic:

thread '

' panicked at 'called Option::unwrap() on a None value', /home/rustbuild/src/rust-buildbot/slave/stable-dist-rustc-linux/build/src/libcore/option.rs:362
playpen: application terminated with error code 101

Playground URL: https://play.rust-lang.org/?gist=13ac9c8daa26803dd005&version=stable
Gist URL: https://gist.github.com/13ac9c8daa26803dd005

@dotdash
Copy link
Contributor

dotdash commented Jun 11, 2015

That panic seems to result from the playground environment. thread::sleep_ms() fails with a bad syscall error. That does not happen locally for me.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 11, 2015

here's a version without sleep_ms (use debug mode!) http://is.gd/ksb279

// because Rust guarantees thread-safety, we can safely assume x hasn't been changed

how did you come to that conclusion? you released the lock to the mutex by leaving the scope of the first mutex guard. Therefore another thread was able to lock the mutex and modify the inner content. That has nothing to do with thread safety, this is perfectly fine.

@nikomatsakis
Copy link
Contributor

Clearly the problem is that the term "thread safety" is not a formal term with a precise definition. What Rust guarantees is an absence of data-races, nothing more, nothing less. (That said, the std lib does make some effort to make other kinds of errors somewhat harder to come across, such as access to "inconsistent" data that can result from panics, but there are no guarantees there).

@nikomatsakis
Copy link
Contributor

Actually, after some discussion with @aturon, we agreed that Rust gives you more than data-race freedom, but it's hard to succinctly state the property that it gives you. In particular, we guarantee you that something like:

x.f = 3;
print(x.f);

always prints 3, which Java does not, even if both guarantee low-level data-races (in Java's case, through the VM, in Rust's case, through the type system).

@nikomatsakis
Copy link
Contributor

Another way to phrase it is that we allow you to define the "consistency boundaries" -- unless you opt-in using unsafe impl Send or some such, you don't really have to think about threading, except in so far as you use types (like a concurrent hashmap...) that are explicitly allowing parallel access. Anyway, have to mull over how best to phrase this. :)

@aturon
Copy link
Member

aturon commented Jun 11, 2015

FWIW, I agree that the current wording on the main page ("guarantees thread safety") is probably a bit too strong. It used to just talk about data races, but we felt that was too weak -- it didn't really capture the full benefits of Send/Sync. Hopefully we can find an equally concise, but more precise statement. Failing that, I'd be more comfortable with understating rather than overstating (even if we included an asterisk).

We can always make the home page bullets link to various of the blog posts we've been publishing to explain Rust's vision in more detail, and that can help clarify the extent of the benefits of each.

@huonw
Copy link
Member

huonw commented Jun 11, 2015

NB. Rust does actually provide tools that allow this sort of code to be guaranteed-correct. The problem here is there's high-level semantics imposed on a plain integer, which is far too general for the compiler to know if the code is correct (for all the compiler knows it may be fine for the value to change between the two critical sections). Using a custom type allows imposing higher-order constraints. In this case, moving (instead of copying) is enough to guarantee the semantics you want:

use std::sync::{Arc, Mutex};
use std::thread;

#[derive(PartialEq, Eq)]
pub struct Value {
    x: i32
}
impl Value {
    pub fn new(x: i32) -> Value { Value { x: x } }

    pub fn increment(&mut self) { self.x += 1; }
}

fn main() {
    let p = Arc::new(Mutex::new(Value::new(0)));

    for _ in 1..100 {
        let x = p.clone();
        thread::spawn(move || {
            let n;
            {
                let mut x_ = x.lock().unwrap();
                x_.increment();
                n = *x_;
            }
            thread::sleep_ms(50);
            {
                // because Rust guarantees thread-safety, we can safely assume x hasn't been changed
                let x_ = x.lock().unwrap();
                if *x_ != n {
                    panic!();
                }
            }
        });
    }

    thread::sleep_ms(2000);
}
<anon>:24:21: 24:24 error: cannot move out of borrowed content
<anon>:24                 n = *x_;
                              ^~~

https://play.rust-lang.org/?gist=782b015afec0d2ea63bc&version=stable&run=1

Don't get me wrong: the original code is still doesn't do what you intended (i.e. Rust isn't automatically guarantee the "thread-safety" that that code wants), but still it is nice that it is possible to do so.

@pythonesque
Copy link
Contributor

@nikomatsakis I think Rust's thread safety guarantees shine the strongest in generic code. Specifically: in safe code, any function generic over T (such that parametricity is preserved) cannot be involved a data race through a value of type T. Period. Not even one that doesn't cause memory unsafety (because there isn't any way to avoid causing memory unsafety over a totally generic T without making sure reads and writes through it are atomic, thanks to things like fat pointers, slices, and enums that aren't safe even given guaranteed atomicity for word-sized writes).

As far as I know, the only time Java guarantees anything anywhere near that strong is during final variable initialization, or situations where the VM takes a lock under the hood (e.g. dynamic class loading). Java avoids the aforementioned problems by simply not having such types in the language; C# avoids them (AFAIK) by heavily restricting operations on value types. Both rely on the garbage collector to avoid use after free on pointer dereferences (I think Copy basically satisfies the requirement that this not be an issue, though). Rust neither avoids nor restricts its value types in this manner, and lacks a garbage collector (and will always lack a general concurrent one, AFAIK), so it has to work harder to achieve the same guarantees.

Of course, there is nothing stopping people from writing an unsafe trait that relaxes these guarantees for types where data races are memory safe; indeed, such a trait would be quite useful! However, no such trait exists currently (that I'm aware of) and requiring it violates parametricity. Moreover, for any trait that doesn't guarantee "this type is safe in the presence of races" (i.e. all existing unsafe traits that I am currently aware of, and all safe traits), the original reasoning applies; in particular, the claim is still valid forT: Sync and T: Send, one or both of which must be a prerequisite for any concurrent data structure. So in practice, any safe, generic, concurrent data structure in Rust (Mutex<T>, for example!) must enforce the above strong property.

I think this is the key to why it feels like Rust is enforcing something stronger than data race safety. But I have no idea how to express it succinctly. It would help if the Rust standard library did define the hypothetical trait I mention above somewhere (Race?), because it would make the explanation a good deal simpler (but still not simple enough to make a one liner on Rust's home page).

@arielb1
Copy link
Contributor

arielb1 commented Jun 20, 2015

@pythonesque More generally, in Rust (unlike Java/C#) you can't have races (i.e. nondeterministic results due to parallelism) on ordinary fields. You can have (safe) data races if you opt-in using Atomic-s, and other race conditions via Mutex-es, or, of course, concurrent external interfaces (Rust doesn't do anything to prevent the access/open race), but you should always be aware of races when using these.

@steveklabnik steveklabnik added A-docs A-allocators Area: Custom and system allocators and removed A-allocators Area: Custom and system allocators labels Sep 3, 2015
steveklabnik added a commit to rust-lang/prev.rust-lang.org that referenced this issue Nov 5, 2015
As rust-lang/rust#26215 and others have pointed out, "thread safety" is a bad way to phrase this. When talking to people about Rust, the thing they've found most striking is compile-time errors, which is really unique to us, so it seems like a good proxy here.
@steveklabnik
Copy link
Member

Submitted rust-lang/prev.rust-lang.org#213

@steveklabnik
Copy link
Member

we've decided this is WONTFIX rust-lang/prev.rust-lang.org#213 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants