Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: sync/atomic: Add Chan atomic channel #66960

Open
eikemeier opened this issue Apr 22, 2024 · 9 comments
Open

proposal: sync/atomic: Add Chan atomic channel #66960

eikemeier opened this issue Apr 22, 2024 · 9 comments
Labels
Milestone

Comments

@eikemeier
Copy link

eikemeier commented Apr 22, 2024

Proposal Details

Add a generic Chan type, analog to other types:

// A Chan is an atomic channel of type chan T. The zero value is a nil channel.
type Chan[T any] struct {
	/// ...
}

// Load atomically loads and returns the value stored in c.
func (c *Chan[T]) Load() chan T

// Store atomically stores ch into c.
func (c *Chan[T]) Store(ch chan T)

// Swap atomically stores new into c and returns the previous value.
func (c *Chan[T]) Swap(new chan T) (old chan T)

// CompareAndSwap executes the compare-and-swap operation for c.
func (c *Chan[T]) CompareAndSwap(old, new chan T) (swapped bool) 

Sample implementation in fillmore-labs.com/lazydone/atomic.

Motivation

Go has a “zero-value-is-useful” mindset, but structures containing channels need to initialize them lazily. It is a well-known problem for struct types to require non-nil channels. If you do not want to go through a mutex every time a channel is accessed (also, the mutex increases the structure size), candidates are atomic.Value or atomic.Pointer.

The first approach is used by context.WithCancel and requires casting:

	d, _ := c.done.Load().(chan struct{})
	if d == nil {
		c.done.Store(closedchan)
	} else {
		close(d)
	}

The second approach by lazydone.SafeLazy requires some pointer arithmetic:

	if ch := l.done.Swap(&closedChan); ch != nil && ch != &closedChan {
		close(*ch)
	}

Internally, a Go channel is a pointer to a hchan structure. So, the second approach uses a pointer to a pointer, which is wasteful.

A channel (*hchan) can be cast from and to an unsafe.Pointer with:

	var ch chan T
	var ptr unsafe.Pointer
	ch = *(*chan T)(unsafe.Pointer(&ptr))
	ptr = *(*unsafe.Pointer)(unsafe.Pointer(&ch))

But nothing in the Go specification guarantees this.

@gopherbot gopherbot added this to the Proposal milestone Apr 22, 2024
@mateusz834
Copy link
Member

sync/atomic is also missing a Map type, like: atomic.Map[K,V], maybe we should add it too? It has the same issues as with chan.

@eikemeier
Copy link
Author

sync/atomic is also missing a Map type, like: atomic.Map[K,V], maybe we should add it too? It has the same issues as with chan.

I'm not sure whether maps would be useful, since the map itself is not thread safe - you'll need additional synchronization anyways. This is not about “completing” atomic types — there are also no atomic strings or functions — it is about improving something I see actively used: It would make WaitChans (see #16620) easier to implement. It's nothing you can't do now with atomic.Pointer, but avoids the double indirection.

@mateusz834
Copy link
Member

mateusz834 commented Apr 22, 2024

I'm not sure whether maps would be useful, since the map itself is not thread safe - you'll need additional synchronization anyways

You can use a map with atomic without other synchronization primitives, map is considered read only, changes are made via copying the entire map, but not sure whether this is worth adding.

@ianlancetaylor
Copy link
Contributor

Lazy initialization of a channel can be done by

var Ch = LazyChannel[int]() // Ch has type func() chan int
var F() {
    Ch() <- 1
}

where LazyChannel is implemented as

func LazyChannel[E any]() func() chan E {
    return sync.OnceValue[chan E](func() chan E { return make(chan E) })
}

@eikemeier
Copy link
Author

Lazy initialization of a channel can be done by

var Ch = LazyChannel[int]() // Ch has type func() chan int
var F() {
    Ch() <- 1
}

How does this work for zero values?

@ianlancetaylor
Copy link
Contributor

Zero valued channels require an initialization step no matter what you do. With the sync.OnceValue approach you can write the channel variable (or field) as type func() chan int and then initialize it to LazyChannel[int](). With the generic atomic.Chan type in this proposal, you'll still have to do something to create the channel. I agree that it will look different, in that you can have multiple different goroutines initialize the value.

@apparentlymart
Copy link

apparentlymart commented Apr 22, 2024

It seems to me that meeting the "zero value is ready to use" goal requires the lazy channel to be represented entirely by a type (as opposed to an initialized function pointer value) so that it can be declared as a part of whatever wrapping type is trying to provide that guarantee:

type UsesLazyChannel struct {
    ch LazyChannel[string]
}

func (ulc *UsesLazyChannel) SendMessage(msg string) {
    // ulc.ch.Ch() initializes the channel on first call, and returns
    // the same channel for all calls thereafter.
    ulc.ch.Ch() <- msg
}

//////////////////////////////////////////////

type LazyChannel[T] struct {
    // ...
}

func (lc *LazyChannel[T]) Ch() chan T {
    // ...
}

The above (assuming there were some real implementation in place of those ... comments) would then allow sending to a zero-valued UsesLazyChannel, on the assumption that LazyChannel[T].Ch initializes a new channel just in time:

ulc := new(UsesLazyChannel)
ucl.Send("Hello")

The most straightforward implementation of this LazyChannel API that comes to my mind is:

type LazyChannel[T] struct {
    ch chan T
    once sync.Once
}

func (lc *LazyChannel[T]) Ch() chan T {
    lc.once.Do(func () {
        lc.ch = make(chan T)
    })
    return lc.ch
}

Unlike the earlier proposed LazyChannel, this one doesn't require first solving how to call sync.OnceValue only once and then storing the returned function pointer somewhere. Instead, the sync.Once itself deals with that at-most-once initialization. This does (internally) use a mutex during initialization, but only an atomic load for any subsequent calls.

This is slightly more complex than the earlier proposed LazyChannel, but not by much. I'm not sure whether it's complicated and/or subtle enough to justify including in package sync, but to me it feels like a more direct answer to the stated use-case.

I don't see any great way to generalize this for buffered channels with Go's current features, because there's no current way to encode a desired buffered size as a type parameter. The best that comes to mind is to make LazyChannel[T].Ch take a buffer size argument which would be used only on the first call, but that seems like confusing API.

The original proposal avoids that challenge by making it the caller's responsibility to actually make the channel, with sync.Chan only worrying about safely assigning that new channel into place (presumably with CompareAndSwap(nil, newChan)). But it seems like a considerably more "fiddly" design if the only goal is to support the zero value of a wrapping type being ready to use. 🤔

@eikemeier
Copy link
Author

Perhaps I didn’t explain my intention well enough: I know how to make lazy channels. Lazy channels are just a sample use case, and as far as this proposal is concerned only a motivation.

Atomic channels are a low-level construct for library builders. I see atomic channels a lot: pkg context, gRPC-Go, HashiCorp Vault, 2, Benthos, and in 488 GitHub search results with duplicates.

They all use atomic.Value, which takes 16 bytes on a 64-bit system, has a weird 2-phase commit (and load) and requires a cast. atomic.Value also uses a *hchan directly, like this proposal. Using sync.Once instead increases the footprint and makes nice tricks, like on-demand channel swapping, even more complicated.

So, this would improve existing code. It enables no new code, since everything that could be done with atomic.Chan can already be done with atomic.Value, albeit less efficient and less clear.

Given that atomic channels results in 488 matches, one could make a case for string (2.7k), slice (2.5k) or map (1.8k), but I assume that would be better discussed elsewhere.

@apparentlymart
Copy link

Thanks for the extra context, @eikemeier.

FWIW when I was writing my variant of LazyChannel it did occur to me that it could be implemented in terms of your proposal (which I hinted at with the reference to CompareAndSwap at the end), but I stopped thinking hard about it once I realized it could be done with sync.Once.

With the understanding that lazy channels is just one use-case (albeit apparently a major one, since that seems to be what many of those examples are doing), here's one more LazyChannel implementation that actually uses what this proposal describes for comparison's sake, since I think it's helpful to discuss concrete examples of how a proposal might be used:

import "atomic"

type LazyChannel[T] struct {
    ch atomic.Chan[T]
}

func (lc *LazyChannel[T]) Ch() chan T {
    ret := lc.ch.Load()
    if ret == nil {
        return lc.init()
    }
    return ret
}

func (lc *LazyChannel[T]) init() chan T {
    ch := make(chan T)
    swapped := lc.ch.CompareAndSwap(nil, ch)
    if swapped {
        return ch
    }
    return lc.ch.Load()
}

I've never personally had a need for "on-demand channel swapping", so I won't try for an example of that, but it could be useful to have a concrete example of a non-lazy-channel-related use-case to consider too.


I agree that it does seem like this same argument could be made for all Go types that are secretly just pointers to a special data structure, or that involve pointers, to avoid the extra level of indirection. They seem justified in the same sense that atomic.Pointer is justified even though there's also atomic.Value.

While I do agree that those would be separate proposals, it does seem like (as you said) approving this proposal is likely to be justification for approving the others too, since the others seem to have even more existing examples. And now that I understand the proposal isn't just about lazy channels, I think offering all of these variations as low-level building blocks seems justified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

5 participants