Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

proposal: x/sync: create CondMutex, inspired by Abseil #41354

Closed
riking opened this issue Sep 12, 2020 · 8 comments
Closed

proposal: x/sync: create CondMutex, inspired by Abseil #41354

riking opened this issue Sep 12, 2020 · 8 comments
Labels
FrozenDueToAge Proposal
Milestone
Proposal

Comments

@riking
Copy link

riking commented Sep 12, 2020 •
edited

Overview

Create a new type, sync.CondMutex, which extends Mutex with some additional methods that allow it to also act as a condition variable.

interface CondMutexIface {
  // Functions identically to Mutex.Lock().
  Lock()
  // Functions identically to Mutex.Unlock(). If necessary, conditions are checked
  // and notified before returning to the caller.
  Unlock()

  // Await blocks the calling thread until the provided condition becomes true.
  // The condition must be a function of state protected by this mutex. The condition
  // callback will only be called while the mutex is held, and may not attempt to call
  // any other functions on this mutex.
  // It is a run-time error if the mutex is not held by the caller on entry to Await.
  //
  // The Await function is equivalent to the following code using sync.Cond:
  //
  //    c.L.Lock()
  //    ... other code ...
  //    for !condition() {
  //      c.Wait()
  //    }
  //    ... other code, condition is true ...
  //    c.L.Unlock()
  Await(condition func() bool)

  // LockWhen blocks the calling thread until the provided condition becomes true,
  // then locks the mutex and returns. It is a convenience alias for calling Lock()
  // immediately followed by Await().
  LockWhen(condition func() bool)

  // possibly also AwaitWithTimeout:
  AwaitContext(context.Context, func() bool) bool
  // not acceptable in standard library; sync does not already depend on time
}

Background

Abseil's Mutex provides an implicit condition variable integrated into the standard Mutex, as documented here: https://abseil.io/docs/cpp/guides/synchronization#conditional-mutex-behavior

The documentation at that link explains more details of this API.

Goals

Because the CondMutex internally takes care of bookkeeping and calling Signal(), it can eliminate many of the error-prone constructions that arise when writing code using condition variables. Users would no longer need to write the while loop themselves, and the mutex can internally take care of selecting the correct waiter (if any) by checking the condition they are waiting on, eliminating or reducing spurious wakeups.

If this API were available, it would completely eliminate manual usage of sync.Cond from at least two of my programs that I can think of right now.

Unresolved Issues

Functions cannot be compared for equality, so outside of reflection or runtime trickery there is no available mechanism for coalescing identical conditions.

Making the condition a field of the mutex (only allowing one possible condition), and allowing calling code to choose whether to use the condition or not, would solve this issue (allowing good performance benefits) at the cost of only allowing one possible condition, as well as potentially introducing brand-new synchronization pathologies to the ecosystem.
@gopherbot gopherbot added this to the Proposal milestone Sep 12, 2020
@riking
Copy link
Author

riking commented Sep 12, 2020

LockWhen should likely be a helper function, not a method on the type.

@davecheney
Copy link
Contributor

it can eliminate many of the error-prone constructions that arise when writing code using condition variables.

Do you have any supporting evidence that sync.Cond is widely used? Does that data support your concern that it sync.Cond is widely misused or error prone?

@seankhliao
Copy link
Member

see #21165 for earlier discussion on sync.Cond

@ianlancetaylor
Copy link
Contributor

There is rarely a reason to use condition variables in Go. I doubt many programs would benefit from this.

It seems to me that using callbacks trades one kind of error-prone construction for a different kind. Bad things will happen if the callback does anything with the CondMutex, but it would be a very easy mistake to make in practice.

@rsc
Copy link
Contributor

rsc commented Sep 16, 2020

The doc comment seems to be wrong. The "equivalent" Go code says that Await starts by locking the mutex, while the text says that the caller must already hold the mutex. If both were true, Await would be guaranteed to deadlock.

Is there a reason this kind of abstraction cannot be developed in a third-party package first?

@bcmills
Copy link
Contributor

bcmills commented Sep 16, 2020

@davecheney, my experience supports the assessment that sync.Cond is widely misused, at least in those cases where it is used. However, I'm deeply skeptical that an API fusing elements of sync.Cond with sync.Mutex would lead to a reduction in misuse: in particular, the proposed API would still make it far too easy to write accidentally-quadratic code in applications that should be linear.

The section of my concurrency talk on condition variables starts at slide 37. In the talk, I enumerated four specific problems with condition variables: spurious wakeups, forgotten signals, starvation, and unresponsive cancellation. As far as I can tell, this proposal would address, at best, half of those: forgotten signals and unresponsive cancellation. (@riking suggests that it would also address spurious wakeups, but I don't think that's strictly true: the call to Await itself would not return spuriously, but the condition callback would still be re-evaluated spuriously just like with a sync.Cond.)

I would rather we encourage Go users to learn and apply appropriate channel-based alternatives.

@davecheney
Copy link
Contributor

@bcmills thanks, for the record, I’m not in favor of this proposal.

@riking
Copy link
Author

riking commented Sep 17, 2020

Thanks for the additional perspective!

@riking riking closed this as completed Sep 17, 2020
@golang golang locked and limited conversation to collaborators Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal
Projects
None yet
Milestone
Proposal
Development

No branches or pull requests
7 participants
@davecheney @rsc @riking @ianlancetaylor @bcmills @gopherbot @seankhliao