Using goroutines to process values and gather results into a slice

I'm recently exploring Go and how goroutines work confuse me.

I tried to port code I had written before into Go using goroutines but got a fatal error: all goroutines are asleep - deadlock! error.

What I'm trying to do is use goroutines to process items in a list, then gather the processed values into a new list. But I'm having problems in the "gathering" part.

Code:

sampleChan := make(chan sample)
var wg sync.WaitGroup

// Read from contents list
for i, line := range contents {
    wg.Add(1)
    // Process each item with a goroutine and send output to sampleChan
    go newSample(line, *replicatePtr, *timePtr, sampleChan, &wg)
}
wg.Wait()

// Read from sampleChan and put into a slice
var sampleList []sample
for s := range sampleChan {
    sampleList = append(sampleList, s)
}
close(sampleChan)

What's the right way to gather results from goroutines?

I know slices are not threadsafe so I can't have each goroutine just append to the slice.

Solution

Your code is almost correct. There's a couple of problems: first, you're waiting for all the workers to finish before collecting the results, and second your for loop terminates when the channel is closed, but the channel is closed only after the for loop terminates.

You can fix the code by asynchronously closing the channel when the workers are finished:

for i, line := range contents {
    wg.Add(1)
    // Process each item with a goroutine and send output to sampleChan
    go newSample(line, *replicatePtr, *timePtr, sampleChan, &wg)
}

go func() {
    wg.Wait()
    close(sampleChan)
}()

for s := range sampleChan {
  ..
}

As a note of style (and following https://github.com/golang/go/wiki/CodeReviewComments#synchronous-functions), it'd be preferable if newSample was a simple, synchronous function that didn't take the waitgroup and channel, and simply generated its result. Then the worker code would look like:

for i, line := range contents {
    wg.Add(1)
    go func(line string) {
        defer wg.Done()
        sampleChan <- newSample(line, *replicatePtr, *timePtr)
    }(line)
}

This keeps your concurrency primitives all together, which apart from simplifiying newSample and making it easier to test, it allows you to see what's going on with the concurrency, and visually check that wg.Done() is always called. And if you want to refactor the code to for example use a fixed number of workers, then your changes will all be local.