2

I see in Essential Go that using a mutex within a struct is not too straight-forward. To quote from the Mutex Gotchas page:

Don’t copy mutexes

A copy of sync.Mutex variable starts with the same state as original mutex but it is not the same mutex.

It’s almost always a mistake to copy a sync.Mutex e.g. by passing it to another function or embedding it in a struct and making a copy of that struct.

If you want to share a mutex variable, pass it as a pointer *sync.Mutex.

I'm not quite sure I fully understand exactly what's written. I looked here but still wasn't totally clear.

Taking the Essential Go example of a Set, should I be using the mutex like this:

type StringSet struct {
    m map[string]struct{}
    mu            sync.RWMutex
}

or like this?

type StringSet struct {
    m map[string]struct{}
    mu            *sync.RWMutex
}

I tried both with the Delete() function within the example and they both work in the Playground.

// Delete removes a string from the set
func (s *StringSet) Delete(str string) {
    s.mu.Lock()
    defer s.mu.Unlock()
    delete(s.m, str)
}

There will obviously be several instances of a 'Set', and hence each instance should have its own mutex. In such a case, is it preferable to use the mutex or a pointer to the mutex?

torek
  • 448,244
  • 59
  • 642
  • 775

2 Answers2

5

Use the first method (a plain Mutex, not a pointer to a mutex), and pass around a *StringSet (pointer to your struct), not a plain StringSet.

In the code you shared in your playground (that version) :

  • .Add(), .Exists() and .Strings() should acquire the lock,
  • otherwise your code fits a regular use of structs and mutexes in go.

The "Don't copy mutexes" gotcha would apply if you manipulated plain StringSet structs :

var setA StringSet
setA.Add("foo")
setA.Add("bar")

func buggyFunction(s StringSet) {
  ...
}


// the gotcha would occur here :
var setB = setA
// or here :
buggyFunction(setA)

In both cases above : you would create a copy of the complete struct

so setB, for example, would manipulate the same underlying map[string]struct{} mapping as setA, but the mutex would not be shared : calling setA.m.Lock() wouldn't prevent modifying the mapping from setB.

LeGEC
  • 46,477
  • 5
  • 57
  • 104
  • Thanks for the example of the 'gotcha'. I must admit my 'Java-trained' brain prevented me from seeing this. As the NewSet() function returns a pointer to the Set, and all Set methods use pointers, I guess a pointer to a mutex would be redundant indeed. –  Aug 31 '21 at 07:04
-1

If you go the first way, i.e.

type StringSet struct {
    m map[string]struct{}
    mu            sync.RWMutex
}

any accidental or intentional assignment/copy of a struct value will create a new mutex, but the underlying map m will be the same (because maps are essentially pointers). As a result, it will be possible to modify/access the map concurrently without locking. Of course, if you strictly follow a rule "thou shalt not copy a set", that won't happen, but it doesn't make much sense to me.

TL;DR: definitely the second way.

bereal
  • 32,519
  • 6
  • 58
  • 104
  • 1
    The OP's method `Delete` is a pointer receiver - so the mutex will not be copied. So, as you say, don't copy it elsewhere. – colm.anseo Aug 30 '21 at 16:53
  • @colm.anseo that sounds like a weird constraint for a set type. And someone will someday violate it for sure. – bereal Aug 30 '21 at 17:02
  • 2
    Most Go code will use a mutex value to ease in struct value initialization and also prevent accidentally sharing mutexes. If a value receiver is used or the struct value is copied, `go vet` (which runs on any test) will pickup the incorrect usage. – JimB Aug 30 '21 at 17:12
  • 1
    Also, "copying" a set - with the same backing map - seems ripe for more bugs. – colm.anseo Aug 30 '21 at 17:24
  • @colm.anseo I had the same thought at first, until I realized that standard Go containers (maps and slices) behave the same way, and everyone is already used to it. So why would passing a map behave differently from passing a map with a mutex? – bereal Aug 31 '21 at 06:07
  • I think this all boils down to how one would copy `StringSet`. While copying a pointer to a mutex works - copying the map (pointer) is not what one would want IMHO. A `dst := CopyStringSet(src)` function would solve all the inner logic: lock access to `src` map while copying to new map; create new mutex (pointer or not) – colm.anseo Aug 31 '21 at 14:46