CODEWITHSUNDEEP
goconcurrencysynchronizationlocking
Sergii Getman
Sergii Getman

Reputation: 4381

Should we synchronize variable assignment in goroutine?

Let's assume I declared two maps and want to assign it in two different goroutines in error group. I don't perform any reads/write. Should I protect assign operation with lock or I can omit it?

UPD3: In the Java Concurrency In Practice Brian Goetz's Part I Chapter 3 Shared Objects, mentioned:

Locking is not just about mutual exclusion; it is also memory visibility. To ensure that all threads see the most up-to-date values of shared mutable variables, the reading and writing threads must synchronize on a common lock.

var (
    mu     sync.Mutex
    one    map[string]struct{}
    two    map[string]struct{}
)
g, gctx := errgroup.WithContext(ctx)
g.Go(func() error {
    resp, err := invokeFirstService(gctx, request)
    if err != nil {
        return err
    }
    mu.Lock()
    one = resp.One
    mu.Unlock()
    return nil
})

g.Go(func() error {
    resp, err := invokeSecondService(gctx, request)
    if err != nil {
        return err
    }
    mu.Lock()
    two = resp.Two
    mu.Unlock()
    return nil
})
if err := g.Wait(); err != nil {
    return err
}
// UPD3: added lock and unlock section
m.Lock()
defer m.Unlock()
performAction(one, two)

UPD: added more context about variables

UPD2: what were my doubts: we have 3 goroutines - parent and two in the error group. there is no guarantee that our parent goroutine shared memory gets the last update after errgroup goroutines complete until we wrap access to shared memory with memory barriers

Upvotes: 0

Views: 978

Answers (2)

icza
icza

Reputation: 418745

Group.Wait() blocks until all function calls from the Group.Go() method have returned, so this is a synchronization point. This ensures performAction(one, two) will not start before any writes to one and two are done, so in your example the mutex is unnecessary.

g, gctx := errgroup.WithContext(ctx)
g.Go(func() error {
    // ...
    one = resp.One
    return nil
})

g.Go(func() error {
    // ...
    two = resp.Two
    return nil
})

if err := g.Wait(); err != nil {
    return err
}
// Here you can access one and two safely:
performAction(one, two)

If you would access one and two from other goroutines while the goroutines that write them run concurrently, then yes, you would need to lock them, e.g.:

// This goroutine runs concurrently, so all concurrent access must be synchronized:
go func() {
    mu.Lock()
    fmt.Println(one, two)
    mu.Unlock()
}()

g, gctx := errgroup.WithContext(ctx)
g.Go(func() error {
    // ...
    mu.Lock()
    one = resp.One
    mu.Unlock()
    return nil
})

g.Go(func() error {
    // ...
    mu.Lock()
    two = resp.Two
    mu.Unlock()
    return nil
})

if err := g.Wait(); err != nil {
    return err
}
// Note that you don't need to lock here
// if the first concurrent goroutine only reads one and two.
performAction(one, two)

Also note that in the above example you could use sync.RWMutex, and in the goroutine that reads them, RWMutex.RLock() and RWMutex.RUnlock() would also be sufficient.

Upvotes: 4

Pan Ke
Pan Ke

Reputation: 579

In this case, only one goroutine can access the map. I don't think you need a lock.

Upvotes: 0

Related Questions