Skip to content

Rust does not guarantee thread-safety #26215

Closed

Description

@no-longer-on-githu-b

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.

Activity

  1. llogiq commented on Jun 11, 2015

    @llogiq
    Contributor

    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

  2. dotdash commented on Jun 11, 2015

    @dotdash
    Contributor

    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.

  3. oli-obk commented on Jun 11, 2015

    @oli-obk
    Contributor

    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.

  4. nikomatsakis commented on Jun 11, 2015

    @nikomatsakis
    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).

  5. nikomatsakis commented on Jun 11, 2015

    @nikomatsakis
    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).

  6. nikomatsakis commented on Jun 11, 2015

    @nikomatsakis
    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. :)

  7. aturon commented on Jun 11, 2015

    @aturon
    Contributor

    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.

  8. huonw commented on Jun 11, 2015

    @huonw
    Contributor

    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.

  9. pythonesque commented on Jun 13, 2015

    @pythonesque
    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).

  10. arielb1 commented on Jun 20, 2015

    @arielb1
    Contributor

    @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.

  11. added
    A-allocatorsArea: Custom and system allocators
    and removed
    A-allocatorsArea: Custom and system allocators
    on Sep 3, 2015
  12. added a commit that references this issue on Nov 5, 2015
  13. steveklabnik commented on Nov 5, 2015

    @steveklabnik
    Contributor
  14. steveklabnik commented on Nov 10, 2015

    @steveklabnik
    Contributor

    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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions