Go channel infinite loop

Question

I am trying to catch errors from a group of goroutines using a channel, but the channel enters an infinite loop, starts consuming CPU.

func UnzipFile(f *bytes.Buffer, location string) error {
    zipReader, err := zip.NewReader(bytes.NewReader(f.Bytes()), int64(f.Len()))

    if err != nil {
        return err
    }

    if err := os.MkdirAll(location, os.ModePerm); err != nil {
        return err
    }

    errorChannel := make(chan error)
    errorList := []error{}

    go errorChannelWatch(errorChannel, errorList)

    fileWaitGroup := &sync.WaitGroup{}

    for _, file := range zipReader.File {
        fileWaitGroup.Add(1)
        go writeZipFileToLocal(file, location, errorChannel, fileWaitGroup)
    }

    fileWaitGroup.Wait()

    close(errorChannel)

    log.Println(errorList)

    return nil
}

func errorChannelWatch(ch chan error, list []error) {
    for {
        select {
        case err := <- ch:

            list = append(list, err)
        }
    }
}

func writeZipFileToLocal(file *zip.File, location string, ch chan error, wg *sync.WaitGroup) {
    defer wg.Done()

    zipFilehandle, err := file.Open()

    if err != nil {
        ch <- err
        return
    }

    defer zipFilehandle.Close()

    if file.FileInfo().IsDir() {
        if err := os.MkdirAll(filepath.Join(location, file.Name), os.ModePerm); err != nil {
            ch <- err
        }
        return
    }

    localFileHandle, err := os.OpenFile(filepath.Join(location, file.Name), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, file.Mode())

    if err != nil {
        ch <- err
        return
    }

    defer localFileHandle.Close()

    if _, err := io.Copy(localFileHandle, zipFilehandle); err != nil {
        ch <- err
        return
    }

    ch <- fmt.Errorf("Test error")
}

So I am looping a slice of files and writing them to my disk, when there is an error I report back to the errorChannel to save that error into a slice.

I use a sync.WaitGroup to wait for all goroutines and when they are done I want to print errorList and check if there was any error during the execution.

The list is always empty, even if I add ch <- fmt.Errorf("test") at the end of writeZipFileToLocal and the channel always hangs up.

I am not sure what I am missing here.

Answer 1

1. For the first point, the infinite loop:

Citing from golang language spec:

A receive operation on a closed channel can always proceed immediately, yielding the element type's zero value after any previously sent values have been received.

So in this function

func errorChannelWatch(ch chan error, list []error) {
    for {
        select {
        case err := <- ch:

            list = append(list, err)
        }
    }
}

after ch gets closed this turns into an infinite loop adding nil values to list.

Try this instead:

func errorChannelWatch(ch chan error, list []error) {
    for err := range ch {
            list = append(list, err)
    }
}

2. For the second point, why you don't see anything in your error list:

The problem is this call:

errorChannel := make(chan error)
errorList := []error{}

go errorChannelWatch(errorChannel, errorList)

Here you hand errorChannelWatch the errorList as a value. So the slice errorList will not be changed by the function. What is changed, is the underlying array, as long as the append calls don't need to allocate a new one.

To remedy the situation, either hand a slice pointer to errorChannelWatch or rewrite it as a call to a closure, capturing errorList.

For the first proposed solution, change errorChannelWatch to

func errorChannelWatch(ch chan error, list *[]error) {
    for err := range ch {
            *list = append(*list, err)
    }
}    

and the call to

errorChannel := make(chan error)
errorList := []error{}

go errorChannelWatch(errorChannel, &errorList)

For the second proposed solution, just change the call to

   errorChannel := make(chan error)
   errorList := []error{}

   go func() {
      for err := range errorChannel {
          errorList = append(errorList, err)
      }
   } () 

3. A minor remark:

One could think, that there is a synchronisation problem here:

fileWaitGroup.Wait()

close(errorChannel)

log.Println(errorList)

How can you be sure, that errorList isn't modified, after the call to close? One could reason, that you can't know, how many values the goroutine errorChannelWatch still has to process.

Your synchronisation seems correct to me, as you do the wg.Done() after the send to the error channel and so all error values will be sent, when fileWaitGroup.Wait() returns.

But that can change, if someone later adds a buffering to the error channel or alters the code.

So I would advise to at least explain the synchronisation in a comment.