go vet range variable captured by func literal when using go routine inside of for each loop

Question

I'm not really sure what a 'func literal' is thus this error is confusing me a bit. I think I see the issue - I'm referencing a range value variable from inside a new go routine thus the value might change at anytime and not be what we expect. What's the best way to solve the issue?

The code in question:

func (l *Loader) StartAsynchronous() []LoaderProcess {
    for _, currentProcess := range l.processes {
        cmd := exec.Command(currentProcess.Command, currentProcess.Arguments...)
        log.LogMessage("Asynchronously executing LoaderProcess: %+v", currentProcess)
        go func() {
            output, err := cmd.CombinedOutput()
            if err != nil {
                log.LogMessage("LoaderProcess exited with error status: %+v\n %v", currentProcess, err.Error())
            } else {
                log.LogMessage("LoaderProcess exited successfully: %+v", currentProcess)
                currentProcess.Log.LogMessage(string(output))
            }
            time.Sleep(time.Second * TIME_BETWEEN_SUCCESSIVE_ITERATIONS)
        }()
    }
    return l.processes
}

My proposed fix:

func (l *Loader) StartAsynchronous() []LoaderProcess {
    for _, currentProcess := range l.processes {
        cmd := exec.Command(currentProcess.Command, currentProcess.Arguments...)
        log.LogMessage("Asynchronously executing LoaderProcess: %+v", currentProcess)
        localProcess := currentProcess
        go func() {
            output, err := cmd.CombinedOutput()
            if err != nil {
                log.LogMessage("LoaderProcess exited with error status: %+v\n %v", localProcess, err.Error())
            } else {
                log.LogMessage("LoaderProcess exited successfully: %+v", localProcess)
                localProcess.Log.LogMessage(string(output))
            }
            time.Sleep(time.Second * TIME_BETWEEN_SUCCESSIVE_ITERATIONS)
        }()
    }
    return l.processes
} 

But does that really solve the problem? I've just moved the reference from the range variable to a different local variable whose value is based off of the iteration of the for each loop that I'm in.

Answer 1

Don't feel bad it's a common mistake for new comers in Go, and yes the var currentProcess changes for each loop, so your goroutines will use the last process in the slice l.processes, all you have to do is pass the variable as a parameter to the anonymous function, like this:

func (l *Loader) StartAsynchronous() []LoaderProcess {

    for ix := range l.processes {

        go func(currentProcess *LoaderProcess) {

            cmd := exec.Command(currentProcess.Command, currentProcess.Arguments...)
            log.LogMessage("Asynchronously executing LoaderProcess: %+v", currentProcess)

            output, err := cmd.CombinedOutput()
            if err != nil {
                log.LogMessage("LoaderProcess exited with error status: %+v\n %v", currentProcess, err.Error())
            } else {
                log.LogMessage("LoaderProcess exited successfully: %+v", currentProcess)
                currentProcess.Log.LogMessage(string(output))
            }

            time.Sleep(time.Second * TIME_BETWEEN_SUCCESSIVE_ITERATIONS)

        }(&l.processes[ix]) // passing the current process using index

    }

    return l.processes
}

Answer 2

For those looking for a simpler example:

This is wrong:

func main() {
  for i:=0; i<10; i++{
    go func(){
        processValue(i)
    }()
  }
}

func processValue(i int){
  fmt.Println(i)
}

Is not exactly an error but could lead to unexpected behavior since the variable i, which controls your loop could be changed from other go routine. It's actually go vet command which alerts about this. Go vet helps precisely to find this kind of suspicious constructs, it uses heuristics that do not guarantee all reports are genuine problems, but it can find errors not caught by the compilers. So it's a good practice to run it from time to time.

Go Playground runs go vet before running the code, you can see that in action here.

This is correct:

func main() {
  for i:=0; i<10; i++{
    go func(differentI int){
        processValue(differentI)
    }(i)
  }
}

func processValue(i int){
  fmt.Println(i)
}

I intentionally named the func literal parameter differentI to make it obvious that it is a different variable. Doing it that way is safe for concurrent use, go vet won't complain and you'll get no weird behaviors. You can see that in action here. (You will see nothing since the printing is done on different go routines but the program will exit successfully)

And by the way, a func literal is basically an anonymous function :)

Answer 3

Yes, what you did is the simplest way of fixing this warning properly.

Before the fix, there was only a single variable, and all goroutines were referring to it. This means they did not see the value from when they started but the current value. In most cases this is the last one from the range.

Answer 4

If you have experience with Javascript & closures in JS, it is very similar & easy to compare it with the classic example for setTimeout inside of a for loop with the setTimeout using the final value of looping var at the end of the iteration.