crystal-lang
The return value of Atomic::Flag#test_and_set is backwards.
The semantics of the test-and-set instruction was established back in 1951 on the EDSAC2 and has been propagated in many implementations since: an atomic operation that tests a flag, sets it, and returns the result of test ... that is, it returns the previous value of the flag. (See https://en.wikipedia.org/wiki/Test-and-set and https://en.cppreference.com/w/cpp/atomic/atomic_flag/test_and_set)
Unfortunately, the semantics of Atomic::Flag#test_and_set is "Atomically tries to set the flag. Only succeeds and returns true if the flag wasn't previously set; returns false otherwise."
This seems to be based on a misapprehension that setting the flag is something that can fail (it can't) and the caller wants to know about that, when in fact the flag is set unconditionally, and the caller wants to know the prior state of the flag. The return value of Atomic::Flag#test_and_set is the exact opposite of what it should be, returning true if it was previously false and returning false if it was previously true. Both the code and the documentation are in error.
The revision path for this sort of mistake is painful ... first introduce a newly named function, perhaps set_and_return_previous_value or corrected_test_and_set, while deprecating test_and_set, then in a later release eliminate test_and_set, then in a yet later release rename the correct function to test_and_set. Fortunately test_and_set is not likely to have a lot of current users (if any). [I didn't encounter this as a user; I was just reading the documentation.]
What you're explaining is the behavior of Atomic(Bool)#swap. It will return the old value.
What you're explaining is the behavior of
Atomic(Bool)#swap. It will return the old value.
Please read what I wrote, and look at the code, instead of being so casually dismissive ... it compares the return value of swap to false, thereby inverting it. And again, the documented behavior is the inverse of what it should be.
You expect the C/C++ behavior, but it's not how its implemented and documented in Crystal. There's no documented underlying value for the flag. We simply report whether the flag has changed from unset to set (return true) or not (return false).
Atomically tries to set the flag. Only succeeds and returns true if the flag wasn't previously set; returns false otherwise.
flag = Atomic::Flag.new
flag.test_and_set # => true # flag wasn't set
flag.test_and_set # => false # flag has been set already
flag.clear
flag.test_and_set # => true # flag has been cleared
flag.test_and_set # => false # flag has been set already
Now, there's not much point in this type anymore, even as a low-level primitive; Atomic(Bool) or a custom Atomic(T) allow to choose the memory order (e.g. acquire then release), while Atomic::Flag is stuck to be sequentially consistent.
I'd consider deprecating it.
@ysbaddaden Yes, that's the behaviour of swap(true). But according to the linked references (e.g. Wikipedia's definition: "CPU instruction to set a memory location to 1 and return its prior value"), that's exactly what test_and_set is usually expected to do.
Our implementation, however, returns the inverse: it is equivalent to !swap(true).
That can be confusing to users who are familiar with the concept of test_and_set in other languages.
If you raelly want to use it: transform the while flag.test_and_set; spin; end into until flag.test_and_set; spin; end.
Deprecating sounds good. It seems to be no longer necessary since #14532.
Its methods just delegate to swap(true) and set(false) on Array(Bool).
EDIT: Fixed swap(false) to set(false).
Its methods just delegate to swap(true) and swap(false) on Array(Bool)
Nit: they delegate to swap(true) and set(false) on Atomic(Bool)
That can be confusing to users who are familiar with the concept of
test_and_setin other languages.
Not just other languages, but the entire literature on test-and-set spanning nearly 75 years. I first learned about it in my CS studies nearly 60 years ago, and presumably it's still being taught when discussing synchronization primitives.
The current backward semantics of Atomic::Flag#test_and_set should certainly be deprecated one way or another, and since Atomic::Flag is just a wrapper around an Atomic(Bool) with very limited methods on it, then deprecating the whole struct does seem like the cleanest and least painful path.
On the other hand: it's quite nice to have a high-level interface with clearly labelled operations and a focused API. Atomic::Flag avoids dealing with the low-level interaction. test_and_set and clear are easier to use than the equivalent plumbing methods.
Ok, but that leaves the problem that the semantics and implementation of Atomic::Flag#test_and_set are wrong and backwards, and fixing it would require a multi-stage deprecation and correction process. Suggesting transforming while flag.test_and_set ... into until flag.test_and_set ... in user code misses the point, but one possible approach is to deprecate the current name and replace it with acquired? ... that would return true if the flag was false and false if the flag was true--the same semantics that test_and_set currently has.
Yes, of course. There's no neat way to transform to correct semantics. It would require a hard break of some sort. I'm just saying that maybe we'd want to find a way to offer this nice high-level API again (either now or in the future).
One solution could be to deprecate Atomic::Flag and introduce a new version with fixed semantics under a different name, for example Atomic::Bool.
We're cross-posting ... see my edit that I just posted: "one possible approach is to deprecate the current name and replace it with acquired? ... that would return true if the flag was false and false if the flag was true--the same semantics that test_and_set currently has."
It seems to me that Atomic::Bool is too similar to Atomic(Bool) while having quite different intent and semantics. Perhaps Atomic::Lock or Atomic::Spinlock with an acquire? method per above? This becomes an exercise in bikeshedding.
Reviewing usages in stdlib, we use Atomic::Flag to do something once in a few places, or to avoid doing something repeatedly until we can again (see @interrupted in the epoll/kqueue event loops).
None need special memory ordering, they just need the op to be atomic. There could be an Atomic::Once but that feels overkill for the few cases we have.
See #15805 for an example.