Broken atomics puzzle

1

A friend of mine discovered this today. Consider this code:

use std::sync::atomic::{ATOMIC_BOOL_INIT, AtomicBool, Ordering};

pub const REPO_LOCK: AtomicBool = ATOMIC_BOOL_INIT;

fn main() {
    let working = REPO_LOCK.compare_and_swap(false, true, Ordering::SeqCst);
    println!("REPO_LOCK = {}", working);
    let working = REPO_LOCK.compare_and_swap(false, true, Ordering::SeqCst);
    println!("REPO_LOCK = {}", working);
}

It is expected to print false then true, but it doesn't.

Do you see why :wink: ?

7 Likes
2

It's a const instead of a static. Maybe an opportunity for a lint of some sort.

9 Likes
3

Isn't this a bug in the definition of AtomicBool::compare_and_swap()? In particular, it takes &self as a parameter, but it should take &mut self instead. It wasn't noticed because compare_exchange() also has the same problem.

4

Hm, isn't it precisely the point of atomic types to provide interior mutability? If you require &mut self, then you can issue compare_and_swap from one thread only.

5

Right, but it's still surprising to a novice when &T does not confer some level of immutability.

6

That's why I've sometimes wished for three reference types:

  • &T for truly immutable data (not available now)
  • &mut T for shared, but potentially interior-mutating access (what is now &T)
  • &uniq T for unique mutable access (what is now &mut T)

Getting good, intuitive borrow-checker semantics for these could be headache-inducing, though.

3 Likes
7

It definitely helps if you think of &T as "shared reference" instead of "immutable reference." For most types, mutation through shared references is unsafe, but atomic operations (AtomicBool et al.), locks (Mutex, RwLock), run-time checks (RefCell), and/or compile-time checks (all of the above, plus Cell) can be used to make it safe.

6 Likes
8

The surprising thing is that rustc is apparently letting you take &self on a interior-mutable const here, whereas it does stop you if you do this at a global level.

error[E0492]: cannot borrow a constant which contains interior mutability, create a static instead
 --> src/main.rs:4:36
  |
4 | static REPO: &'static AtomicBool = &REPO_LOCK;
  |                                    ^^^^^^^^^^

But I guess what's happening with REPO_LOCK.compare_and_swap(...) is that it's making a local copy of the constant, and then referencing that for the &self call. It's pretty similar to what happens with mutating a literal constant, like let x = &mut 42; *x += 1;.

1 Like
9

No, AtomicBool and the other atomic types are intended to be updated concurrently via & (shared) references. If they required &mut then there would be no point using them over a plain value.

The issue here is that a const creates a new copy of itself every time it's used, even if the value's type is not Copy (it's kind of magic like that). It's like if you stuck a .clone() before each method call.

Conversely, static marks a value as a global which you can take references to and even update in-place if it contains UnsafeCell (which all the atomics do), which is what @matklad's friend intended.

10

This error should probably extend to this situation because it's such an obvious footgun. It might be pertinent to open an issue on Github.

1 Like
11

The point of atomic types is to have atomic updates, not interior mutability. UnsafeCell is "the core primitive for interior mutability in Rust," so I would have expected that any interior mutability should be done inside an UnsafeCell and if that mutability needs to be atomic, then there would be an atomic type inside the UnsafeCell.

But, it's too late to argue that because it's already been decided to do something different.

However, this example seems to show the type system is unsound. After all, any change to a constant needs to be forbidden by the definition of “constant.” So some fix to the type system is needed.

12

This isn't even specific to interior mutability or shared/immutable borrows. This code also compiles:

const NUMBER: u8 = 5;
NUMBER.add_assign(1);
println!("{}", NUMBER); // prints 5
4 Likes
13

@briansmith I don't see what this has to do with soundness - the example above essentially turns into

use std::sync::atomic::{ATOMIC_BOOL_INIT, AtomicBool, Ordering};

fn main() {
    let working = ATOMIC_BOOL_INIT.compare_and_swap(false, true, Ordering::SeqCst);
    println!("REPO_LOCK = {}", working);
    let working = ATOMIC_BOOL_INIT.compare_and_swap(false, true, Ordering::SeqCst);
    println!("REPO_LOCK = {}", working);
}

That's just how consts work - they're more like #defines than statics.

14

How the compiler currently compiles const and the semantics of const are not necessarily the same. In particular, see https://github.com/Kimundi/rfcs/blob/rvalue_static_promotion/text/0000-rvalue_static_promotion.md, particularly its references to UnsafeCell:

The UnsafeCell restrictions are there to ensure that the promoted value is truly immutable behind the reference.

15

I'm not sure I understand what rvalue promotion has to do with this?

16

The semantics of const are outlined here, by the way: https://github.com/rust-lang/rfcs/blob/master/text/0246-const-vs-static.md#constants

They seem pretty explicit to me that this case would behave exactly as expected.

17

I misunderstood. The atomic types are wrappers around UnsafeCell and so the problem I thought was going to occur isn't an issue, AFAICT.

But, then, doesn't that RFC also provide the key to resolving this: Just don't allow const values to use UnsafeCell constructors, similar to how that RFC handles UnsafeCell?

18

I don't think "just" is the right word to describe a proposal that would at the minimum break all code in existence that uses ATOMIC_FOO_INIT.

19

You're right. Like @mbrubeck demonstrated, it's not about interior mutability anyway.