Refactor by applying the "Mutex hat" idiom

[alexandear]

The PR improves readability by applying the "Mutex hat" idiom. See also this slide.

Additionally, this PR renames mutexes names by using the Mu suffix.

[jandubois]

I'm wondering if it would make sense to group protected fields together with their mutex into their own struct. That would avoid repeating some of the redundant naming, and makes it even more obvious which fields are protected by which mutex.

Something like this (untested):

type worthCheckingIPTables struct {
	sync.RWMutex
	value bool
}

type latestIPTables struct {
	sync.RWMutex
	entries []iptables.Entry
}

type agent struct {
	worthCheckingIPTables
	latestIPTables
}

func foo(a *agent) {
	a.worthCheckingIPTables.Lock()
	a.worthCheckingIPTables.value = true
	a.worthCheckingIPTables.Unlock()
}

This is only meant as a RFC, I'm not sure if this works better or not.

[alexandear]

I'm not sure if we will gain any benefit by adding a struct that contains a mutex and protected fields. We have only 7 mutexes in the repo, which is a small amount.

The Go codebase doesn't introduce a separate struct for mutexes, so we shouldn't either.

[jandubois]

The Go codebase doesn't introduce a separate struct for mutexes, so we shouldn't either.

Just briefly browsing those search results it looks like almost half of them show a struct with a mutex at top (most often called just mu) that protects the whole struct.

An exact example of the pattern I'm discussing here can be seen at https://github.com/golang/go/blob/81c92352a7c6aadc434e7d0921d046a599ba2807/src/os/signal/signal.go#L13.

[alexandear]

On the other hand, Uber Go Style Guide is against embedding mutexes:

Exception: Mutexes should not be embedded, even on unexported types.

The Goland may warn about embedded mutexes when the team implements https://youtrack.jetbrains.com/issue/GO-14143.

[jandubois]

On the other hand, Uber Go Style Guide is against embedding mutexes:

Exception: Mutexes should not be embedded, even on unexported types.

The Goland may warn about embedded mutexes when the team implements https://youtrack.jetbrains.com/issue/GO-14143.

I honestly don't understand the argument how foo.Mu.Lock() is good but foo.Lock() is bad. In both cases you expose the fact that you need to lock the struct before you should access the fields. And it is not an internal implementation detail; it is part of the contract on how you can use the struct.

I do get the argument that you should use one style consistently, but it seems arbitrary which one you pick.

Anyways, I'm fine with naming the mutext mu and not use embedding. I just don't want someThingVeryLongAndHardToRead and another someThingVeryLongAndHardToReadMu field that I have to lock.

[jandubois]

I honestly don't understand the argument how foo.Mu.Lock() is good but foo.Lock() is bad.

Ok, I do get the argument if all the protected fields are non-exported fields. In that case you should not expose the locking mechanism either. But as soon as any protected fields are exported, you must export the locking mechanism too.

And none of this should apply to non-exported types.