proposal: sync: add Mutex.Do

[dsnet]

I propose the following helper method being added to Mutex:

// Do runs f under the protection of the mutex.
func (m *Mutex) Do(f func()) {
    m.Lock()
    defer m.Unlock()
    f()
}

Rationale

I was investigating a sluggish program and the result was because defer m.Unlock() is function scoped.

Consider the following snippet:

func (s *Server) doSomething() {
    s.mu.Lock()
    defer s.mu.Unlock()

    ... // access some critical resource protected by mu

    ... // hundreds of lines of logic that doesn't need protection by mu
}

Initially when doSomething was written it was concise and short such that s.mu.Lock() and the corresponding defer s.mu.Unlock() concisely protected the body of the function. However, as usually goes with software engineering, this function grew in complexity such that logic was added that doesn't care about the resource protected by s.mu. We are now unnecessarily holding the mutex for much longer than necessary (since defer s.mu.Unlock() doesn't run until the function returns). This problem gets worse over time as the unrelated logic grows since the s.mu operations get push father away (in terms of code locality) hiding it's existence and the runtime complexity of the unrelated logic grows as well.

With a Do method, the scope of the critical region becomes clear:

func (s *Server) doSomething() {
    s.mu.Do(func() {
        ... // access some critical resource protected by mu
    })

    ... // hundreds of lines of logic that doesn't need protection by mu
}

[jimmyfrasche]

The same would apply to RLock. Would that be called RDo? I suggest WithLock and WithRLock.

[dsnet]

For RWMutex, Do and RDo sound fine.

[dsnet]

Alternatively, we could do:

func WithLock(lock Locker, f func()) {
    lock.Lock()
    defer lock.Unlock()
    f()
}

func WithLockValue[T any](lock Locker, f func() T) T {
    lock.Lock()
    defer lock.Unlock()
    return f()
}

func WithLockValues[T1, T2 any](lock Locker, f func() (T1, T2)) (T1, T2) {
    lock.Lock()
    defer lock.Unlock()
    return f()
}

This avoids methods on sync.Mutex and sync.RWMutex, but makes the call pattern more complex:

m.Do(func() { ... })

versus

sync.WithLock(&m, func() { ... })

However, it has the advantage that we can declare the Value and Values variant that returns one or two arguments similar to the existingOnceValue and OnceValues functions.

[baryluk]

Already supported.

func(){
  m.Lock()
  defer m.Unlock()
  ...
}()

You can use it in locks, or in shorter sections of a function, to scope it shorter.

[earthboundkid]

An alternative design for sync/v2 would be to have Mutex protect generic values like this: https://github.com/carlmjohnson/syncx/blob/main/mutex.go It’s more like Rust in that you can’t misuse the protected value.

[baryluk]

@carlmjohnson This is not good. It is too restrictive. You often need to access more complex types (i.e. maps) while holding a mutex, do read-modify-write operation, change multiple values, keep lock held, when calling a function, etc. Such wrapper to protect one variable is only a trouble. From my experience in Go, C++, D, such generic protected value, is more trouble than a solution, and in majority of cases is not enough.

[dolmen]

I have seen Mutex and RWMutex directly embedded in structs to expose directly the Lock and Unlock methods.

type X struct {
    sync.Mutex
    // ...
}

This isn't a good practice if the type is public, but we must consider how adding a method would impact those cases.

Edit: You can find many such cases in private types in stdlib:

$ grep -lR '\tsync.Mutex$' src | wc -l
      26

[earthboundkid]

@carlmjohnson This is not good. It is too restrictive. You often need to access more complex types (i.e. maps) while holding a mutex, do read-modify-write operation, change multiple values, keep lock held, when calling a function, etc.

In the design of my package, you would write m.Lock(func(v *Val) { /* do stuff */ }) for a multi-step scenario. I would say the big flaw of this approach is that it is necessarily a shallow lock. E.g. if you stored a map in the mutex, some could just save a copy of the map and do a racey write outside of the lock function.

[DeedleFake]

An alternative design for sync/v2 would be to have Mutex protect generic values like this: https://github.com/carlmjohnson/syncx/blob/main/mutex.go It’s more like Rust in that you can’t misuse the protected value.

I quite like this design and think it's overall a better solution to the problem being addressed by this proposal, but I think that it should be an alternative to the current sync.Mutex design, not a full replacement for it. They can each be useful. sync.Value[T], perhaps?

It is notable, though, that make(chan T, 1) is pretty much functionally the exact same thing, except that a mutex-style implementation could have a useful zero value, and could possibly be a bit better in terms of performance.

[merykitty]

I feel this is just a poor man's workaround for scoped defer, the issue here seems to be that there is nothing to separate the critical region from the non-critical one, which leads to code being added accidentally into the critical region.

[dolmen]

Here is an alternate naming scheme for the sync.WithLock from @dsnet above:

func Locked[L Locker](lock L, f func()) {
  lock.Lock()
  defer lock.Unlock()
  f()
}

type RLocker interface {
  RLock()
  RUnlock()
}

func RLocked[L RLocker](lock L, f func()) {
  lock.RLock()
  defer lock.RUnlock()
  f()
}

Usage:

sync.Locked(&m, func() {
  // ...
})
sync.RLocked(&m, func() {
  // ...
})

https://go.dev/play/p/qV6MVksBZjA

[baryluk]

If these functions are so simple, then you can add it to your project, and start using them. They do not add much value being in a standard library.

[adonovan]

Shouldn't it be a "push" iterator of type func (m *Mutex) Do(f func() bool) so that you can express your critical section as a block?

for range mu.Do {
    ... critical section ...
}

(Rubyists, I'm kidding.)

[seankhliao]

it does feel a bit like singleflight.Do without the sharding and result reuse

[seankhliao]

func() {
  mu.Lock()
  defer mu.Unlock()
  // ...
}()

I think we should do this, people already frequently use the above pattern as demonstrated by 10k hits on github Do should be able to make the intention clearer and encourage proper scoping.