"Channel size is one or none" forbids common and useful patterns

[peterbourgon]

Channel Size is One or None

Although I definitely appreciate and agree with the intent here, the rule as expressed forbids common patterns, such as the semaphore:

semaphore := make(chan struct{}, 10)
var wg sync.WaitGroup
for i := 0; i < 100; i++ {
	wg.Add(1)
	go func(i int) {
		defer wg.Done()
		semaphore <- struct{}{}        // acquire
		defer func() { <-semaphore }() // release
		println(i)
	}(i)
}
wg.Wait()

And scatter/gather:

// Scatter
values := []int{1, 2, 3, 4, 5}
c := make(chan int, len(values))
for _, v := range values {
	go func(v int) {
		c <- process(v)
	}(v)
}

// Gather
var sum int
for i := 0; i < cap(c); i++ {
	sum += <-c
}

Perhaps "Channels should be unbuffered by default"? There's nothing particularly special about a capacity of 1, I don't think, and all of the provided rationale still applies.

[prashantv]

We often use channels with size 1 for notifications that something has changed, while the actual change is stored separately. The channel itself usually doesn't store anything meaningful (often just chan struct). The primary advantage of a channel vs sync.Cond is the ability to select on it within a channel. E.g.,

Background goroutine:

for {
  select {
  case <-notifyCh:
    // process state stored separately
  case <-time.After(..):
    // some periodic operations
  case <-ctx.Done():
    // exit
  }

Operation to trigger background goroutine:

// mutate state for background goroutine

// notify the background goroutine
select {
case notifyCh <- struct{}{}:
default:
}

So I don't think we should change the recommendation to unbuffered channels only.

However, there are some use-cases where a specifically sized channel makes sense (typically sized as some concurrency limit), so we'll need to figure out how to cover those.

[abhinav]

To add to that, part of the intent behind the recommendation is to advice
readers to be very deliberate and thoughtful about channel buffer sizes. What
we don't want is something like this:

results := make(chan result, 1000) // should be enough
for _, item := range items {
    go func(item *Item) {
        results <- process(item)
    }(item)
}

With an unbuffered channel, or a channel with a deliberately chosen buffer
size, what happens when a send or receive instruction blocks is a question that
is front and center, or closer to it.

[peterbourgon]

Background goroutine:

for {
    select {
    ...
    case <-time.After(...):

Well, that's a leak 😬 Presumably you want a <-ticker.C here?

We often use channels with size 1 for notifications that something has changed,

Sure, that's a pattern, too — I just see it as roughly the same as the others I mentioned, both in utility and in frequency. I'm surprised to hear it's common for you; Go itself is async where it matters, so it's usually a mistake to "async all the things" in your application code, too. It's like re-implementing TCP on TCP, right? 😅 Backpressure is good!

What we don't want is ... results := make(chan result, 1000)

Oh, yes, definitely. Cargo-culting "async is better" or whatever seems to worm its way into everything.

Anyway, your call of course 👍

[prashantv]

case <-time.After(...):

Well, that's a leak Presumably you want a <-ticker.C here?

This was just a compressed example to indicate that it's waiting on other things. That said, time.After only "leaks" till the timer fires,

The underlying Timer is not recovered by the garbage collector until the timer fires. If efficiency is a concern, use NewTimer instead and call Timer.Stop if the timer is no longer needed.

You may be thinking of time.Tick which does leak forever.

Sure, that's a pattern, too — I just see it as roughly the same as the others I mentioned, both in utility and in frequency. I'm surprised to hear it's common for you; Go itself is async where it matters, so it's usually a mistake to "async all the things" in your application code, too. It's like re-implementing TCP on TCP, right? Backpressure is good!

This pattern works quite well when work can be coalesced.

[peterbourgon]

That said, time.After only "leaks" till the timer fires,

Yes, but, that definitely counts :) If your time.After is 1s, and you get 100k sends on your notify channel in 1s, then you've got 100k time.Timers sitting around occupying memory. Hopefully you got a linter that catches this bug.

You may be thinking of time.Tick which does leak forever.

Yep, that one's even worse!