Function returns lock by value

Question

I have the following structure

    type Groups struct {
        sync.Mutex
        Names []string
    }

and the following function

    func NewGroups(names ...string) (Groups, error) {
        // ...
        return groups, nil
    }

When I check for semantic errors with go vet, I am getting this warning:

NewGroups returns Lock by value: Groups

As go vet is shouting, it is not good. What problems can this code bring? How can I fix this?

Banjo

Answer 1

You need to embed the sync.Mutex as a pointer:

type Groups struct {
    *sync.Mutex
    Names []strng
}

Addressing your comment on your question: In the article http://blog.golang.org/go-maps-in-action notice Gerrand is not returning the struct from a function but is using it right away, that is why he isn't using a pointer. In your case you are returning it, so you need a pointer so as not to make a copy of the Mutex.

Update: As @JimB points out, it may not be prudent to embed a pointer to sync.Mutex, it might be better to return a pointer to the outer struct and continue to embed the sync.Mutex as a value. Consider what you are trying to accomplish in your specific case.

Essentialism

One would normally return a *Groups instead of embedding a ponter. – Notus

Update: As @Notus points out, it may not be prudent to embed a pointer to sync.Mutex This is too bold. both solution, in regard to that specific field, are valid. Such decision is built upon the shape of the type supported by that mutex. – Biopsy

Answer 2

Return a pointer *Groups instead.

Embedding the mutex pointer also works but has two disadvantages that require extra care from your side:

1. the zero value of the struct would have a nil mutex, so you must explicitly initialize it every time

func main() {
    a, _ := NewGroups()
    a.Lock() // panic: nil pointer dereference
}

func NewGroups(names ...string) (Groups, error) {
    return Groups{/* whoops, mutex zero val is nil */ Names: names}, nil
}

2. assigning a struct value, or passing it as function arg, makes a copy so you also copy the mutex pointer, which then locks all copies. (This may be a legit use case in some particular circumstances, but most of the time it might not be what you want.)

func main() {   
    a, _ := NewGroups()
    a.Lock()
    lockShared(a)
    fmt.Println("done")
}

func NewGroups(names ...string) (Groups, error) {
    return Groups{Mutex: &sync.Mutex{}, Names: names}, nil
}

func lockShared(g Groups) {
    g.Lock() // whoops, deadlock! the mutex pointer is the same
}

Keep your original struct and return pointers. You don't have to explicitly init the embedded mutex, and it's intuitive that the mutex is not shared with copies of your struct.

func NewGroups(names ...string) (*Groups, error) {
    // ...
    return &Groups{}, nil
}

Playground (with the failing examples): https://play.golang.org/p/CcdZYcrN4lm

Trier