5 ms·

re: "# channels < # goroutines" func doSomethingTwice() error { // Issue occurs below errc := make(chan error) go func() { def

by shoo 8y ago

re: "# channels < # goroutines" func doSomethingTwice() error { // Issue occurs below errc := make(chan error) go func() { defer fmt.Println("done wth a") errc <- doSomething("a") }() go func() { defer fmt.Println("done with b") errc <- doSomething("b") }() err := <-errc return err } > When one routine writes to the channel, the program exits and the other goroutine is lost, building up memory use as a results I'm not very familiar with go, but my guess would be that since the two goroutines send to an unbuffered channel that is read at most once, the "slower" of the two goroutines will sit there blocking attempting to send to a channel that will never be read. So this would "leak" a goroutine that would consume resources while the process was still running, but it doesn't have anything to do with the program exiting. https://golang.org/ref/spec#Channel_types https://golang.org/ref/spec#Channel_types https://golang.org/ref/spec#Send_statements https://golang.org/ref/spec#Send_statements > How do we fix this? We simply increase the number of channels to 2 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.

SamReidHughes 8y ago
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.
- shoo 8y ago
  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
  - seabee 8y ago
    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.
geocar 8y ago
> 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.
- cdoxsey 8y ago
  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.
nilsocket 8y ago
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. }
nemothekid 8y ago
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 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.

by shoo 8y ago

SamReidHughes 8y ago

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.

shoo 8y ago
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
- seabee 8y ago
  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.

geocar 8y ago

> 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.

cdoxsey 8y ago
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.

nilsocket 8y ago

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. }

nemothekid 8y ago

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 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.