Mutex ignored on root level of a struct? [Solved]

[marczahn]

Hi,

I have a struct with receiver methods and a mutex:

type DatabaseService struct {
	sync.Mutex
	client *ts.DatabaseServiceClient
	auth   *ts.Auth
	cache map[string]*CachedDatabaseConfig
}
//...
func (s DatabaseService) GetDatabase(ownerUUID uuid.UUID, name string) (*Database, error) {
	s.Lock()
	defer s.Unlock()
	cacheKey := ownerUUID.String() + "." + name
	logEntry := logrus.WithField("owner_uuid", ownerUUID.String()).WithField("name", name)
	if c, ok := s.cache[cacheKey]; ok {
		// ...
	}
     // ...

Strange thing now is that the mutex lock is ignored. Normally all calls except the first one should stop at if c, ok := s.cache[cacheKey]; ok { But it does not

On the other hand here it is working:

type DatabaseConfigCache struct{
    sync.Mutex
    items map[string]*CachedDatabaseConfig
}

type DatabaseService struct {
    client *ts.DatabaseServiceClient
    auth   *ts.Auth
    cache *DatabaseConfigCache
}

// ...
func (s DatabaseService) GetDatabase(ownerUUID uuid.UUID, name string) (*Database, error) {
    s.cache.Lock()
    defer s.cache.Unlock()
    cacheKey := ownerUUID.String() + "." + name
    logEntry := logrus.WithField("owner_uuid", ownerUUID.String()).WithField("name", name)
    if c, ok := s.cache.items[cacheKey]; ok {

Can anyone tell me why the mutex on the first example is ignored.

btw: I tested it with 100 parallel go routines that accessed this function

[NobbZ (Norbert Melzer)]

On a quick glance it seens to be a by-value vs. by-ref issue.

In most cases you want to have *sync.Mutex.

There was a linting tool that warns you in such cases.

[calmh (Jakob Borg)]

Also, consider not embedding. Do you want users of the DatabaseService to call the Lock and Unlock methods? If not, something like this might be what you are looking for:

type DatabaseService struct {
    mut sync.Mutex // zero value is usable, like you do today
    ...
}

func (s *DatabaseService) GetDatabase(...) { // note, pointer receiver
    s.mut.Lock()
    defer s.mut.Unlock()
    ...
}

[NobbZ (Norbert Melzer)]

In general I try to even abstract the sync.Mutex completely away behind setters and getters and unexported fields.

According to my experience this will save a lot of time searching for deadlocks or races because of forgotten mutex (un)locks. Or delayed execution of some go-routines because a defered unlocks in other goroutines which happen far “to late”.

In some cases it might be necessary to get and set many values from a receiver, which are behind a single lock. In such cases I do often provide some kind of batch environment. But so far it was necessary only once.

[dfc (Dave Cheney)]

Go vet will help here. TLDR, place all your methods on *DatabaseService to avoid copying the mutex and locking the copy.

[marczahn]

Hey dfc - You are absolutely right. I overlooked this. Thank you for this hint!

[dfc (Dave Cheney)]

There is a great rule of thumb in gopl.io, which goes something like "if you have a method on *T, then all the methods should be on *T", basically, don’t mix an match, pick one and be consistent.

A side effect of this rule is it tends to drive you to place methods on *T, not T because if you have a method that needs to mutate T, you need to put the method on *T so you’re not mutating a copy of the receiver.

[marczahn]

You are right - Not using a pointer of the receiver at this place was just a mistake I overlooked.