Map concurrent usage

Question

I came across this piece of code and was wondering if this needs to have a R/W Mutex.

method(){
var (
    wg           sync.WaitGroup
    rwm          sync.RWMutex
    vcnRegionMap map[string][]core.Vcn
)

vcnRegionMap = make(map[string][]core.Vcn)

// This loops helps us in filtering unused regions
// for composition of region/vcnid ds
for _, regionName := range regions {

    wg.Add(1)
    go func(ctx context.Context, region string, vcnRegionMap map[string][]core.Vcn, wg *sync.WaitGroup, rwm *sync.RWMutex) {
        // for locking maps

        defer wg.Done()
        // TODO: make this conditional if a region is specified
        c.network.SetRegion(region)

        vcnResponse, err := c.network.ListVcns(ctx, core.ListVcnsRequest{
            CompartmentId: &c.cID,
        })
        if err != nil {
            logger.Debug(err.Error())
        }
        if len(vcnResponse.Items) == 0 {
            logger.Info("status 404: No Vcns found under the given OCID and region: %s", region)
            return
        }
        
        logger.Info("status 200: Vcns found under the given OCID and region: %s", region)
        for _, item := range vcnResponse.Items {
            logger.Debug("Vcn object: %s", *item.DisplayName)
            // maps are not concurrency safe
            rwm.Lock()
            defer rwm.Unlock()
            vcnRegionMap[region] = append(vcnRegionMap[region], item)
        }

    }(ctx, regionName, vcnRegionMap, &wg, &rwm)
}
wg.Wait()
}

As each go routine gets it's own copy of map, does the Mutex help in anyway and can we avoid it to reduce latency?

I don't see anywhere the `vcnRegionMap` map is being copied. You must always synchronize concurrent modifications of any value. — JimB, Oct 05 '20 at 16:04
@JimB Please correct me if wrong. My understanding is that since the anonymous function is not a closure and an explicit parameter is passed to go routine, it will have it's own copy of the map to work with. — f-z-N, Oct 05 '20 at 16:08
A copy of the map header is passed to the function, not a deep-copy of the map. Each goroutine is still modifying the same underlying map. — Burak Serdar, Oct 05 '20 at 16:10
wonder why downvotes for trying to understand some piece of code. Just discourages people from asking! — f-z-N, Oct 05 '20 at 16:20
@f-z-N: if it's for your own understanding, then votes don't matter. It will likely end up downvoted, because it's not that useful to the site as a whole to repeat answers about this topic so frequently. — JimB, Oct 05 '20 at 16:27
thank you @JimB. I guess one last question in this regard. Does it then make any sense to pass the map as a parameter to the routine. Would it be rather better to form a closure and remove the clutter from function def — f-z-N, Oct 05 '20 at 16:46
@f-z-N, You can do it either way. Personally I would remove the parameters to eliminate confusion because 3 out of 4 parameters are effectively closed over. You must ensure that the `regionName` is copied still however, which I would do with the `regionName := regionName` idiom. — JimB, Oct 05 '20 at 16:50

score 3 · Accepted Answer · answered Oct 05 '20 at 16:09

3

You need to protect the map from being concurrently accessed. The code is wrong because you are read-locking the mutex, but writing to the map.

 for _, item := range vcnResponse.Items {
            logger.Debug("Vcn object: %s", *item.DisplayName)
            // maps are not concurrency safe
            rwm.Lock()
            vcnRegionMap[region] = append(vcnRegionMap[region], item)
            rwm.Unlock()
        }

Note that this version does not use defer. Deferred operations run when the function returns, not when the block ends. You read-lock the mutex n times, once for each iteration, and then release all of them when the function returns.

answered Oct 05 '20 at 16:09

Burak Serdar

46,455
3
40
59

@JimB, would it really be ok? It is still a write to the map. You wouldn't corrupt the map, but it would be racy without a lock. – Burak Serdar Oct 05 '20 at 16:27
Yes, you're correct, it is technically a write, even if the key is the same. The question was not about the slice at all, but that also has a race that might be worth mentioning. – JimB Oct 05 '20 at 16:31

Map concurrent usage

1 Answers1