Is it OK to defer an Unlock before a Lock

Question

I'm going over some existing code and see this repeated several times

defer mtx.Unlock()
mtx.Lock()

This looks wrong to me, I much prefer the idiomatic way of deferring the Unlock after performing the Lock but the documentation for Mutex.Lock doesn't specify a situation where the Lock will fail. Therefor the behaviour of the early defer pattern should be identical to the idiomatic way.

My question is: is there a compelling case to say this pattern is inferior? (e.g. the Lock may fail and then the deferred Unlock will panic) and thus the code should be changed or should I leave it as is?

Answer 1

Short answer:

Yes, it's OK. The defer call is made after the function returns (well, sort of).

The longer, more nuanced answer:

It's risky, and should be avoided. In your 2 line snippet, it's not going to be a problem, but consider the following code:

func (o *Obj) foo() error {
    defer o.mu.Unlock()
    if err := o.CheckSomething(); err != nil {
        return err
    }
    o.mu.Lock()
    // do stuff
}

In this case, the mutex may not be locked at all, or worse still: it may be locked on another routine, and you end up unlocking it. Meanwhile yet another routine obtains a lock that it really shouldn't have, and you either get data races, or by the time that routine returns, the unlock call will panic. Debugging this kind of mess is a nightmare you can, and should avoid at all cost.

In addition to the code looking intuitive and being more error prone, depending on what you're trying to achieve, a defer does come at a cost. In most cases the cost is fairly marginal, but if you're dealing with something that is absolutely time critical, it's often better to manually add the unlock calls where needed. A good example where I'd not use defer is if you're caching stuff in a map[string]interface{}: I'd create a struct with the cached values and an sync.RWMutext field for concurrent use. If I use this cache a lot, the defer calls could start adding up. It can look a bit messy, but it's on a case by case basis. Either performance is what you aim for, or shorter, more readable code.

Other things to note about defers:

• if you have multiple defers in a function, the order in which they are invoked is defined (LIFO).
• Defers can alter the return values of the function you end up calling (if you use named returns)