sync/atomic: add (*Value).Swap and (*Value).CompareAndSwap

[carnott-snap]

Between a recent use case, and previous tickets (#11260, #20164, #26728), there is renewed interest in implementing Swap and CompareAndSwap for sync/atomic.Value.

The new interface serves to be more ergonomic, eschew the use of unsafe, and make atomic.Value have parity with the atomic.XxxPointer methods. There are some key differences between how the atomic.Value.XxxSwap and atomic.XxxSwapPointer work, e.g. panic on inconsistent types or nil, but these are fundamental to how atomic.Value works, and thus an accepted complexity.

Interface definition follows:

package atomic

// Swap stores new into Value and returns the previous value. It returns nil if
// the Value is empty.
//
// All calls to Swap for a given Value must use values of the same concrete
// type. Swap of an inconsistent type panics, as does Swap(nil).
func (v *Value) Swap(new interface{}) (old interface{})

// CompareAndSwapPointer executes the compare-and-swap operation for the Value.
//
// All calls to CompareAndSwap for a given Value must use values of the same
// concrete type. CompareAndSwap of an inconsistent type panics, as does
// CompareAndSwap(nil, old).
func (v *Value) CompareAndSwap(new, old interface{}) (swapped bool)

[ianlancetaylor]

See also #11260, #20164 and #26728.

[carnott-snap]

See also #11260, #20164 and #26728.

Thanks for the pointers; I did look, but it appears searching is hard with the wrong keywords.

One thing that is not called out in the linked tickets is the warning on the unsafe package. My use case does work with atomic.SwapPointer, but I am unwilling to accept the risks presented by the unsafe package:

Packages that import unsafe may be non-portable and are not protected by the
Go 1 compatibility guidelines.

[oguzyildizz]

In my case, I need atomic.Swap because I have a variable that collects batched statistics (many go routines write to it), and one go routine is responsible of replacing that batch with a new empty batch atomically and export the old one. I had to use atomic.SwapPointer to swap and atomic.LoadPointer to get it from the writers because Mutexes had too much lock contention.

[bcmills]

Note: we could also call this atomic.Value.Swap. It would be the same implementation, but with a name more closely aligns with atomic.SwapPointer.

I don't understand this comment. Swap is not at all the same operation as LoadOrStore — LoadOrStore is more like a special case of CompareAndSwap, where the value to be compared is “no value stored”.

[bcmills]

I do think there might be some value in a (*atomic.Value).LoadOrStore, though — in some cases, I think that could help users to avoid panics resulting from the “same concrete type” restriction, particularly if the value being stored is some concrete implementation of the error interface.

[bcmills]

One thing that is not called out in the linked tickets is the warning on the unsafe package.

Note that #20164 specifically calls out avoidance of unsafe as one of the main motivating factors.

Also note that both #20164 and #26728 were withdrawn for lack of compelling concrete use-cases, so it would be very helpful to see some concrete examples of where the proposed method would be used.

[carnott-snap]

I don't understand this comment. Swap is not at all the same operation as LoadOrStore — LoadOrStore is more like a special case of CompareAndSwap, where the value to be compared is “no value stored”.

That is fair and I may have gotten carried away with being compatible to sync.Map and company. I am open to either LoadOrStore, Swap, or CompareAndSwap, as they all meet my needs.

Note that #20164 specifically calls out avoidance of unsafe as one of the main motivating factors.

atomic.Value seems to be the preferred replacement for the atomic.*Pointer methods in code that avoids package unsafe.

According to @bradfitz in CL 41930, unsafe.Pointer is strongly discouraged outside of the sync, runtime, and reflect packages.

While the above is called out, it felt like a preference and did not mention the non-portable and compatibility implications. My apologies if this was obvious subtext in #20164.

Also note that both #20164 and #26728 were withdrawn for lack of compelling concrete use-cases, so it would be very helpful to see some concrete examples of where the proposed method would be used.

This was submitted on behalf of @oguzyildiz1991's use case. Let me know if more detail would be beneficial. Was the resolution of other tickets that unsafe.Pointer was good enough?

[rsc]

The resolution on #26728 said:

There aren't many environments that reject unsafe anymore, and we shouldn't contort the library for those.

It would be fine for someone who has a compelling use for these to create a new issue proposing to add Swap and CompareAndSwap (or to reopen this one, if you have permission). For now, given the lack of a real-world use case for them, we will err on the side of leaving it out.

Is there a new real-world use case where Value.Swap is helpful?

[oguzyildizz]

how about the one I described above? @rsc

[carnott-snap]

There aren't many environments that reject unsafe anymore, and we shouldn't contort the library for those.

My concern is less about the inability to use unsafe, and more my unwillingness. If I import "unsafe", I loose the Go 1 compatibility guarantee. I think it is untenable to say that you need to do that just to use the sync/atomic package.

[bcmills]

@carnott-snap, if you're worried about formal compatibility policies in conjunction with sync/atomic, you might also take a look at #5045. (sync/atomic itself doesn't have well-described behavior yet, although we have a fairly clear picture of what the definition of that behavior should be.)

[carnott-snap]

While I am now also concerned about the exact details of the sync/atomic package, it is still covered under the Go 1 compatibility guarantee. It seems like sync/atomic's use of unsafe.Pointer is stable, and could be covered by the guarantee, but since the Pointer symbol is in the unsafe package, you take a risk.

To be clear, I am arguing that using atomic.CompareAndSwapPointer (or LoadPointer, StorePointer, SwapPointer) is unsafe, thus the argument made in previous tickets, that atomic.Value.Swap is not required, is untenable. There exist other solutions, like exposing a safe atomic.Pointer or accepting interface{}, but I think atomic.Value.Something is the most straightforward.