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