Hacker News Reader

> The fix is okay but the language is a bit hazy, there's still only one channel, but now it's a buffered channel with capacity to hold up to 2 messages, so the two goroutines don't need to block waiting for a receiver to be ready to synchronously receive the message they are sending.

I was very disappointed with this explanation as well. The documentation is less than direct on this point, and suggests goroutines "execute independently", so my only conclusion is that the author doesn't understand channels very well, and was perhaps led to not understand them well.

    01  func doSomethingTwice() error {
    02      // Issue occurs below
    03      errc := make(chan error)
    04
    05      go func() {
    06          defer fmt.Println("done wth a")
    07          errc <- doSomething("a")
    08      }()
    09      go func() {
    10          defer fmt.Println("done with b")
    11          errc <- doSomething("b")
    12      }()
    13      err := <-errc
    14      return err
    15  }

If the programmer understands the control flow is something like: 03, 06, 07, 10, 13, 14 then they're not going to be confused, and they're certainly not going to "fix it" by increasing the buffer, they'll fix it by reading from the channel twice, and guarding against this by closing the channel. The real question is how to explain to the programmer what the semantics of channels is:

A goroutine blocks on read or write of an unbuffered channel (what we're seeing here).

The garbage collector is for simulating an infinite memory machine only. It is not for "cleaning up" after you.

The issue comes up when go programmers learn about runtime.GOMAXPROCS much too early, so they learn goroutines as threads, and they guess that the thread will be cleaned up when "garbage collected" because that's how Python works (or some other "garbage collected" language they might be familiar with). They're further confused because golang values have a "finalizer" so they might just assume that the thread finalizer (somehow) kills the thread, or the channel finalizer (somehow) closes the channel.

Perhaps if they noticed that writing to a closed channel causes a run-time panic, they might think these semantics are unlikely.

The point about buffering is to be able to write twice without blocking. Yes, you get 03, 06, 07, 10, 11, 13, 14 but how important was that really? If you did it just because you don't want to "leak memory", then you still don't know what's going on.

Increasing the channel size to 2 is the correct fix.

You don't want to read twice because then you would block waiting for both messages.

When the channel size is two the two inner writing goroutines can succeed on their write to the channel even if nothing reads from it.

The channel has three references, the outer receiver, and the two inner senders. The receiver finishes as soon as its able to receive the first message.

So what happens is one of the messages is sent and received, the other is sent and just sits in the channel. Then the channel has no more references and is garbage collected, so there's no memory leak.

Also goroutines are scheduled onto threads. So these orphaned goroutines that can't complete eat up memory (their call stack + the channel which can't be completed) as well as put pressure on the scheduler. (though the scheduler is efficient and can handle millions of goroutines) That's why it makes sense to refer to this as a memory leak.

> Increasing the channel size to 2 is the correct fix.

Or is the correct fix to use errgroup[1]?

  func doSomethingTwice() error {
  	var g errgroup.Group
  	g.Go(func() error {
  		defer fmt.Println("done wth a")
  		return doSomething("a")
  	})
  	g.Go(func() error {
  		defer fmt.Println("done with b")
  		return doSomething("b")
  	})
  	return g.Wait()
  }

[1] https://godoc.org/golang.org/x/sync/errgroup

> Increasing the channel size to 2 is the correct fix.

Sometimes. Sometimes it is not. For a trivial example, a unix-style wait would be done by reading twice. A job queue would be another.

It is not, for the reasons specified in the article, which falsely states that the buffer length and the number of goroutines has some relationship (# channels < # goroutines).

The channel size needs to match or exceed the number of producing goroutines. Anything less and the some of the goroutines will block attempting to send to a channel with no receiver.

The example in the article is a common issue that comes up in go programs, and the suggested fix is what is usually recommended.

The other way to solve it would be to read the first error and then up another goroutine to read the remaining error, but that seems more complicated to me.

> The channel [buffer] size needs to match or exceed the number of producing goroutines

This is not true, for the reasons I outlined.

> The example in the article is a common issue that comes up in go programs, and the suggested fix is what is usually recommended.

That may be, but it doesn't change the fact that go programmers that take this advice are creating a kind of cargo-cult around channels and goroutines that will hold them back.

> The other way to solve it would be to read the first error and then up another goroutine to read the remaining error, but that seems more complicated to me.

If all you ever do with channels and goroutines is simulate threads, you're missing out. Erlang programmers might more-easily appreciate this given that they have to go through an incredible contortion to get this feature that go provides for free.

Far better to just read twice from the channel, too. It's generally a smell to leave stuff on a channel. And that's a good example why -- you're losing track of a goroutine with no way to wait for completion or interrupt it.

i see what you mean. it'd be pretty strange to wait and see if you got an error from the first goroutine to complete. i'd imagine usually you'd want to wait for the first non-error result to arrive, say, or for all results. maybe there's a scenario where this kind of thing is useful, but it's probably not common

The two most common scenarios in my 3 years of experience are fan-in and first-error (executing stuff for their side effects only). It’s easy to mess up the latter, but golang.org/x/sync/errgroup is usually what you want.

I don't like this example and solution, because while it doesn't leaks (the channel goes out of scope, and the goroutine ends) it still is 1. error prone you now have to have a buffered channel, 2. sort of wasteful, setting the buffer to 1 instead of 2, also wouldn't leak and 3. not clear.

If you don't care about the results of the goroutine (here you explicitly don't care about the return value of at least one of the routines), a WaitGroup is much better.

    import "sync"
    func doSomethingTwice() error {
      // Issue occurs below
      var wg sync.WaitGroup()
      wg.Add(1)
      go func() {
          defer fmt.Println("done wth a")
          wg.Done()
      }()
      wg.Add(1)
      go func() {
          defer fmt.Println("done with b")
          wg.Done()
      }()
      wg.Wait()
      return nil
    }

The more advanced concept than this, which isn't in the stdlib is errgroup (https://godoc.org/golang.org/x/sync/errgroup#Group.Wait), where instead the accompayning `Wait()` function can also return an error.

In general I consider a code smell if you aren't reading every value off the channel.

A better solution would be a struct as return value of channel which contains both value and error as fields.

So just casually one would handle the case:

if ret.err != nil{

       //Handle error.


}