Why is this function not thread safe in golang?

Question

This is the code that I am mentioning:

// this is inside some method which has return signature like this: (*Data, error)
mapStore := make(...)
resSlice := make(...)
wg := new(sync.WaitGroup)
ec := make(chan error)
for keyString, sliceValue := range myMap {
     wg.Add(1)

     keyString := keyString
     sliceValue := sliceValue

     go func() {
          err := func(keyString string, sliceValue []Value, wg *sync.WaitGroup) error {
                defer wg.Done()
                res, err := process(keyString, sliceValue)
                if err != nil {
                      return errors.Wrapf(err, "wrong")
                }
                if res == nil {
                      return nil
                }
                if res.someData != nil {
                      mapStore[*res.someData] = append(mapStore[*res.someData], res)
                      return nil
                }
                resSlice := append(resSlice, res)
                return nil
               }(keyString, sliceValue, wg)
               if err != nil {
                     ec <- err
                     return
               }
          }()
     }
}
wg.Wait()

select {
case err := <- ec:
      return nil, err
default:
      return resSlice, nil
}

I am told that this is not thread safe for some reason but I am not confident as to where. I am thinking its the issue in handling err in ec but would appreciate some help!

Answer 1

I haven't analyzed all of it, but definitely the modification of mapStore from multiple goroutines is unsafe:

mapStore[*res.someData] = append(mapStore[*res.someData], res)

But as a starting point, run this under the race detector. It'll find many problems for you.

This is also clearly unsafe:

resSlice := append(resSlice, res)

But it also doesn't quite do what you think. This creates a new local variable called resSlice that shadows the outer one, but it also modifies the outer one (see comments below). Beyond the fact that two things may try to append at the same time and collide, append can move the whole slice in memory if it needs to reallocate, so this can cause thread-safety issues even if you put locks around it.

Typically, rather than having each goroutine update some central variable, you want to have each goroutine pass its results back on a channel. Then have the main function collect all the values and update the variables. See Go Concurrency Patterns: Pipelines and cancellation for some examples.