`Arc::strong_count` memory ordering is a potential footgun

[lukas-code]

In #115546 the memory ordering of {Arc,Weak}::{strong,weak}_count got changed to Relaxed, which can cause some hard to debug race bugs.

I tried this code:

use std::cell::UnsafeCell;
use std::sync::Arc;
use std::thread;

struct SendMe<T>(T);
unsafe impl<T> Send for SendMe<T> {}

fn main() {
    let a = Arc::new(UnsafeCell::new(0i32));
    let b = SendMe(a.clone());
    thread::spawn(move || {
        let b = b;
        for _ in 0..100 {
            unsafe { *b.0.get() += 1; }
        }
    });
    while Arc::strong_count(&a) != 1 {}
    // core::sync::atomic::fence(core::sync::atomic::Ordering::Acquire);
    unsafe { *a.get() += 1; }
}

I expected to see this happen: I expected the check strong_count == 0 to be strong enough to avoid a data race in this program. My intuition was that this check implies that no other thread can access the data concurrently.

Instead, this happened: With the fence commented out, the above program has undefined behavior. Miri outputs:

error: Undefined Behavior: Data race detected between (1) Write on thread `<unnamed>` and (2) Read on thread `main` at alloc826+0x10. (2) just happened here
  --> src/main.rs:19:14
   |
19 |     unsafe { *a.get() += 1; }
   |              ^^^^^^^^^^^^^ Data race detected between (1) Write on thread `<unnamed>` and (2) Read on thread `main` at alloc826+0x10. (2) just happened here
   |
help: and (1) occurred earlier here
  --> src/main.rs:14:22
   |
14 |             unsafe { *b.0.get() += 1; }
   |                      ^^^^^^^^^^^^^^^
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
   = note: BACKTRACE (of the first span):
   = note: inside `main` at src/main.rs:19:14: 19:27

I think we should either put the Acquire ordering back or add a warning to the docs that such code requires a manual fence.

Meta

1.74 = current beta

cc @SUPERCILEX author of #115546

@rustbot label T-libs A-atomic

[SUPERCILEX]

This can definitely be added to the docs, but it was intentional. The argument is that you shouldn't be synchronizing on the strong_count (you can't write a correct if strong == 1 { finalize } else { drop } since two threads could end up in drop which means you need busy waiting to synchronize I think), so you're left with cases where you don't need Aquire semantics (under the assumption that busy waiting is bad). If it turns out many people are doing busy waiting maybe we'd want to revert?

---

I know your code is probably convoluted for an example, but I'd argue you should use unsafe { *Arc::into_inner(a).unwrap().get() += 1; }. Or even better if you don't need to do the last action on the starting thread, use into_inner on both threads and let whoever comes last do the final action.

[lukas-code]

My use case was something like if strong == 1 { unsynchornized access } else { synchronized access } as a performance optimization, where an acquire load would be sufficient synchronization. I realized now that get_mut can be used in this situation, however that method uses three atomic accesses whereas only one would be required if it is known that no Weak references exist.
Essentially, I'm trying to implement a get_mut_assume_no_weak.

Regarding busy waiting, the code would still be wrong even if we used some kind of synchronized waiting inside inside the loop (example), since the final relaxed load and the non-atomic access after can be reordered. To fix the code, we need a barrier between the final Arc::strong_count and the non-atomic access.

[SUPERCILEX]

Essentially, I'm trying to implement a get_mut_assume_no_weak.

Hmmm, this makes me wonder if there should be an arc without weak support. We'd need to check what other methods have their performance hindered by weak references.

---

Overall, I think the use case makes sense, but having counts be relaxed is more flexible. I'd maybe argue using a fence is clearer code too. My main concern would be performance but as far as I understand a good microarchitecture won't have any trouble with an acquire fence.

[olekspickle]

This change has actually broken one of our older services that was getting a long overdue update and was relying on strong_count. I've spent days trying to figure out the reason refusing to believe Arc was responsible.
With Ordering::Relaxed the behavior became incredibly volatile.
I'll try to create MVE but not sure it'll be possible since I was able to catch this only on loaded prod instance.

[mgeier]

Just to add another data point: the change to Relaxed broke a function in my wait-free ring buffer crate rtrb: mgeier/rtrb#114.

I'm not using that function myself, and I don't know if anyone is, so it might not be a problem at all.

I was planning to replace Arc with some hand-written alternative anyway, so now I have a good reason to prioritize that.

[SUPERCILEX]

Would docs suggesting an aquire fence if the count is equal to 1 help? (Assuming unsynchronized reads at that point.)

The is_abandoned() function seems like a good example of where this change can lead to better performance (since you only need a fence when the other thread is dead which will happen once as opposed to a bunch of times when it's not dead).

[mgeier]

Would docs suggesting an aquire fence if the count is equal to 1 help?

I'm not very familiar with the use of fences, so I'm not sure ...

Can you please clarify what you mean?

In my case, the count is only ever 2 or 1. If it's 1, it means that the other thread has been abandoned.
If I'm getting the value 1 in an unsynchronized way, it has in fact already been synchronized, otherwise I wouldn't have gotten 1.
If I'm getting the value 2 in an unsynchronized way, it could mean anything.

I tried putting an Acquire fence into my implementation of is_abandoned(), but ThreadSanitizer still reports a data race in my test (https://github.com/mgeier/rtrb/blob/2686a28351db711ebb43c5d0fab671d9ed47a5a7/tests/lib.rs#L141-L156). But maybe I'm using it wrong.

Admittedly, I'm using an Arc for something that can manually be implemented in a simpler way, which I would like to do in the long term, anyway.