Tracking issue for compare_exchange

[alexcrichton]

Tracking issue for rust-lang/rfcs#1443

[alexcrichton]

cc @Amanieu and #30969 (the implementation)

[alexcrichton]

Note that the FCP for this should also take into account that the stable compare_and_swap should be deprecated

[Amanieu]

I'm having second thoughts about the return type of compare_exchange_weak. (bool, T) doesn't seem very user-friendly, maybe we could replace it with an enum?

[huonw]

As @Amanieu says, is there a reason that compare_exchange_weak doesn't/can't return Result? i.e. the method on the various Atomic* types is

fn compare_exchange_weak(&self, current: T, new: T, success: Ordering, failure: Ordering) -> Result<T, T>; 

It seems we forgot about this option on the RFC. Thoughts, @rust-lang/libs?

[Amanieu]

I don't think a Result is appropriate in this case since T is not an error type.

[alexcrichton]

Hm, arguably both methods could return Result<T, T> perhaps? Ok(T) means that it was swapped out and T was previously there, whereas Err(T) would mean that it wasn't swapped out and T was the value previously there.

That'd at least unify the APIs, but @Amanieu is right in that .unwrap() may not work or try! in general because T isn't a first-class error. We could add a wrapper, but that may just be overkill?

[Amanieu]

I agree with @alexcrichton that both methods should return the same enum type for consistency. Note that this would require a breaking change the atomic_cxchg intrinsics, which will affect anyone using them directly. However, I still think we should use a custom enum rather than Result.

For reference, here is what I expect a typical compare_exchange_weak loop to look like (with the current (bool, T) return type):

// Load the initial value
let mut val = ATOMIC.load(Ordering::Relaxed);
loop {
    // Do something with it and determine the new value
    let new = do_something(val);

    // Now attempt to atomically change the value to the new one
    match ATOMIC.compare_exchange_weak(val, new, Ordering::Acquire, Ordering::Relaxed) {
        // Success
        (true, _) => break,

        // Failure, we get the current value back. Use that to try again.
        (false, old) => val = old,
    }
}

[huonw]

The intrinsics do not need to change, the exposed functions can wrap the raw return values up into something more Rusty, like we do for checked arithmetic. This approach is in fact significantly easier than getting the compiler to construct the right enum internally. (Additionally, changing them is entirely unproblematic, they're unstable for a reason)

---

I don't think anyone would use unwrap: the only reason to use a CAS (rather than just a plain old swap) is if you expect failure. And, try! is mostly similar (although I could imagine propagating a CAS-failure up out of a helper function that is called by some main loop). The fact that these core Result-manipulators don't seem to apply possibly argues for a new enum, but this is a succeeded (Ok)/failed (Err) situation, meaning Result doesn't seem entirely inappropriate to me.

(The first sentence makes me realise: it's somewhat unfortunate that we have mem::swap and Atomic...::swap, but this is ..._exchange, but there's probably not much we can do about that. Having another method with both compare and swap in its name seems like it would just be maliciously designed for maximum confusion with the deprecated compare_and_swap.)

Diving into the details, maybe the return value should be Result<(), T> (or, with a custom enum, enum CasResult<T> { Succeeded, Failed(T) }), since one already knows the old value in the successful case, and, it's often no longer needed anyway (as seen in @Amanieu's snippet).

[alexcrichton]

I personally recommended Result for a few reasons:

• We've backed away from custom enums in the past before (towards Result)
• While this isn't necessarily a case for Result-style manipulation, the suite of methods on Result are quite nice
• Result is ubiquitously known, ergonomic to use, and doesn't add any extra overhead in terms of what you cognitively need to understand.

I suspect many compare_exchange calls would just be followed by .is_ok() in the long run. I'd also lean more towards not hiding information where although Result<(), T> is more semantically correct (you've relinquished ownership kinda) I'd still vote for Result<T, T> as we've got the info and someone will want to use it.

[aturon]

cc me

[Amanieu]

Should the swap function be deprecated and replaced with exchange? It is the only function whose name doesn't match the C++11 ones. Additionally it isn't consistent with the new compare_exchange names.

[alexcrichton]

I personally would prefer the name swap to exchange and wouldn't want to cause too much unnecessary churn from this, but good to consider!

[alexcrichton]

🔔 This issue is now entering its cycle-long final comment period for stabilization 🔔

[alexcrichton]

Ah and as a plan of action the libs team proposes:

• Stabilizing these APIs in 1.10
• Deprecating compare_and_swap in 1.12

That way there is always a stable non-deprecated way to do compare and swap!

[alexcrichton]

The libs team discussed this during triage yesterday and the decision was to stabilize

[Amanieu]

It seems that we never got around to deprecating compare_and_swap.