Encapsulating "concurrency safety" in Go Maps

Question

I have a struct, MyStruct, which contains a map. I want to make the access to the map safe for concurrent read and write but I also want to stick to the base Map and not use sync.Map.

For this reason I create on MyStruct methods for insert, delete and fetch which are protected by a mutex. The code looks like this

type MyStruct struct {
    mu    sync.Mutex
    myMap map[string]string
}

func (myStruct *MyStruct) Add(val string) {
    myStruct.mu.Lock()
    myStruct.myMap[val] = val
    myStruct.mu.Unlock()
}

func (myStruct *MyStruct) Remove(val string) {
    myStruct.mu.Lock()
    delete(myStruct.myMap, val)
    myStruct.mu.Unlock()
}

func (myStruct *MyStruct) Fetch(val string) string {
    myStruct.mu.Lock()
    ret := delete(myStruct.myMap, val)
    myStruct.mu.Unlock()
    return ret
}

So far so good.

Some clients of MyStruct though need also to loop through myStruct.myMap and here comes my question. Which is the best design to make concurrent safe also loop operations performed not in methods of MyStruct? Currently I see 2 options

1. Make the map myMap and the mutex mu of MyStruct public and move to the clients the responsibility to make the loop thread safe. This is simple but, somehow, feels like MyStruct does not care too much about its clients
2. Keep everything private and add a method that returns a copy of the map to clients which wants safely play with it. This seems better from an "encapsulation' point of view but, at the same time, sounds a bit heavy

Is there any other possibility? Any suggestion on which design is better?

Answer 1

There is sync.Map, which has all the features you need. The main downside is that it doesn't use static typing (due to the lack of generics in Go). This means that you have to do type assertions everywhere to use it like a regular map. Honestly it may be simplest to just use sync.Map and redeclare all of the methods with static types so that clients don't have to worry about doing the type assertions. If you don't like sync.Map, see my other suggestions.

One improvement to mention first, is to replace sync.Mutex with sync.RWMutex. This allows for multiple read operations to happen concurrently. Then, change Fetch to use mu.RLock() and mu.RUnlock()

For looping through the map:

Safely iterate over each value and execute the callback (keeps lock for entire iteration). Note that due to the locking, you can't call Delete or Add in the callback, so we can't modify the map during iteration. Modifying a map during iteration is otherwise valid, see this answer for how it works.

func (myStruct *MyStruct) Range(f func(key, value string)) {
    myStruct.mu.RLock()
    for key, value := range myStruct.myMap {
        f(key, value)
    }
    myStruct.mu.RUnlock()
}

Here's what the usage would look like

mystruct.Range(func(key, value string) {
    fmt.Println("map entry", key, "is", value)
})

Here's the same, but passing in the map with the callback so that the callback function can modify the map directly. Also changing to regular lock in case iteration makes a modification. Note that now if the callback retains a reference to the map and stores it somewhere, it will effectively break your encapsulation.

func (myStruct *MyStruct) Range(f func(m map[string]string, key, value string)) {
    myStruct.mu.Lock()
    for key, value := range myStruct.myMap {
        f(myStruct.myMap, key, value)
    }
    myStruct.mu.Unlock()
}

Here's an option with cleaner usage, as the locking is carefully managed so you can use your other locking functions in the callback.

func (myStruct *MyStruct) Range(f func(key, value string)) {
    myStruct.mu.RLock()
    for key, value := range myStruct.myMap {
        myStruct.mu.RUnlock()

        f(key, value)

        myStruct.mu.RLock()
    }
    myStruct.mu.RUnlock()
}

Notice that the read lock is always held while the range code executes, but is never held while f executes. This means that the ranging is safe*, but the callback f is free to call any other methods like Delete that require locking.

Footnote: While option #3 has the cleanest usage in my opinion, the main thing to note is that since it doesn't hold a lock continuously for the entire iteration, that means that any iteration can be affected by other concurrent modifications. For example, if you start iterating while the map has 5 keys, and concurrently to this some other code is deleting the keys, you can't say whether or not the iteration will see all 5 keys.