52

I have concurrent goroutines which want to append a (pointer to a) struct to the same slice. How do you write that in Go to make it concurrency-safe?

This would be my concurrency-unsafe code, using a wait group:

var wg sync.WaitGroup
MySlice = make([]*MyStruct)
for _, param := range params {
    wg.Add(1)
    go func(param string) {
        defer wg.Done()
        OneOfMyStructs := getMyStruct(param)
        MySlice = append(MySlice, &OneOfMyStructs)
    }(param)
}
wg.Wait()

I guess you would need to use go channels for concurrency-safety. Can anyone contribute with an example?

jameshfisher
  • 34,029
  • 31
  • 121
  • 167
Daniele B
  • 19,801
  • 29
  • 115
  • 173
  • 4
    I believe the response here answers that question well: http://stackoverflow.com/questions/18467445/working-with-slices-of-structs-concurrently-using-references/18469210#18469210 – Gustavo Niemeyer Aug 28 '13 at 23:02

3 Answers3

55

There is nothing wrong with guarding the MySlice = append(MySlice, &OneOfMyStructs) with a sync.Mutex. But of course you can have a result channel with buffer size len(params) all goroutines send their answers and once your work is finished you collect from this result channel.

If your params has a fixed size:

MySlice = make([]*MyStruct, len(params))
for i, param := range params {
    wg.Add(1)
    go func(i int, param string) {
         defer wg.Done()
         OneOfMyStructs := getMyStruct(param)
         MySlice[i] = &OneOfMyStructs
     }(i, param)
}

As all goroutines write to different memory this isn't racy.

Volker
  • 40,468
  • 7
  • 81
  • 87
  • 8
    It's very interesting your last consideration: in case the size of the slice is known and you are just dealing with pointers to the objects, you don't need to use a concurrency mechanism at all – Daniele B Aug 28 '13 at 23:23
  • 1
    This does not depend on "slice of pointers": It would work also for "slice of MyStruct". Again the code never writes to the same memory. – Volker Aug 28 '13 at 23:35
  • I was assuming that the memory allocation for a pointer is fixed, while the memory allocation for a struct is not fixed. I suppose I am wrong then. – Daniele B Aug 29 '13 at 01:10
  • Hu? What is "fixed"? Any type in Go has a certain memory layout which is determined completely a compile time. No difference between a pointer and something else. – Volker Aug 29 '13 at 04:59
  • I've read that you should usually not have buffering in channels, unless there's a specific need. For your suggestion about the goroutines writing to a shared channel, isn't that a case where you would just deliver synchronously (unbuffered)? – Bjarke Ebert Feb 26 '16 at 13:02
  • @DanieleB Can you elaborate why wouldn't you need a concurrency mechanism at all? How else would I map a slice concurrently if I know the length of the original slice? Looking for a more correct/better approach if it exists. – Abhijit Jun 18 '22 at 13:52
  • @Abhijit if you create a new slice of fixed length and you iterate on it to write each element, then you don't need a concurrency mechanism, because there is no concurrency involved. Each element already has its allocated memory and don't concur with other elements. – Daniele B Jun 19 '22 at 14:25
  • @DanieleB If the `getMyStruct` function in the question is a very slow function and I want to run it for all the elements concurrently instead of sequentially, then I'd need a concurrent mechanism, right? – Abhijit Jun 20 '22 at 05:02
  • @Abhijit if such slice is fixed size and local to the function until all elements are written, then you shouldn't need a concurrent mechanism. If you instead you are writing to a slice which is not local to the function, then you would need a concurrent mechanism. – Daniele B Jun 21 '22 at 10:04
  • This solution do not pass the race detection check by `go build -race` – Cirelli94 Feb 10 '23 at 11:29
27

The answer posted by @jimt is not quite right, in that it misses the last value sent in the channel and the last defer wg.Done() is never called. The snippet below has the corrections.

https://play.golang.org/p/7N4sxD-Bai

package main

import "fmt"
import "sync"

type T int

func main() {
    var slice []T
    var wg sync.WaitGroup

    queue := make(chan T, 1)

    // Create our data and send it into the queue.
    wg.Add(100)
    for i := 0; i < 100; i++ {
        go func(i int) {
            // defer wg.Done()  <- will result in the last int to be missed in the receiving channel
            queue <- T(i)
        }(i)
    }

    go func() {
        // defer wg.Done() <- Never gets called since the 100 `Done()` calls are made above, resulting in the `Wait()` to continue on before this is executed
        for t := range queue {
            slice = append(slice, t)
            wg.Done()   // ** move the `Done()` call here
        }
    }()

    wg.Wait()

    // now prints off all 100 int values
    fmt.Println(slice)
}
chris
  • 4,332
  • 5
  • 41
  • 61
  • Coming from the future: why does the @jimt solution not work? The `wg.Done()` is `defer`red, so it is only called after the value is sent through the channel. What am I missing? – ineiti Mar 02 '21 at 15:27
  • @chris Can I use an unbuffered channel? `queue := make(chan T)` – Pioz Mar 16 '21 at 15:47
  • using two different sync primitives is overkill, keeping only channel will do the job – valichek Aug 07 '22 at 09:55
0

I wanted to add that since you know how many values you are expecting from the channel, you may not need to make use of any synchronization primitives. Just read from the channel as much data as you are expecting and leave it alone:

borrowing @chris' answer

package main

import "fmt"

type T int

func main() {
    var slice []T

    queue := make(chan T)

    // Create our data and send it into the queue.
    for i := 0; i < 100; i++ {
        go func(i int) {
            queue <- T(i)
        }(i)
    }

    for i := 0; i < 100; i++ {
        select {
        case t := <-queue:
            slice = append(slice, t)
        }
    }

    // now prints off all 100 int values
    fmt.Println(slice)
}

The select will block until the channels receives some data, so we can rely on this behaviour to just read from the channel 100 times before exiting.

In your case, you can just do:

package main

func main() {
    MySlice = []*MyStruct{}
    queue := make(chan *MyStruct)

    for _, param := range params {
        go func(param string) {
            OneOfMyStructs := getMyStruct(param)
            queue <- &OneOfMyStructs
        }(param)
    }

    for _ := range params {
        select {
        case OneOfMyStructs := <-queue:
            MySlice = append(MySlice, OneOfMyStructs)
        }
    }
}
smac89
  • 39,374
  • 15
  • 132
  • 179