code review 3775048: sync: Proposal for condition variable support.

sync: condition variable support.

This change introduces a new condition variable implementation which helps solving some well known synchronization problems in a simple way.

[niemeyer, 2011-01-05 02:43:22]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[rsc, 2011-01-05 18:07:34]

I'd prefer not to add this to package sync.
In most of the cases where people use condition variables,
simple channel communication is a better answer,
because it both synchronizes *and* communicates
a value.

Russ

[niemeyer, 2011-01-05 18:27:34]

In the cases where simple channel communication is a better answer,
we have channels, which are great. This proposal, though, addresses
the case where they're not a better answer, and these cases are not
rare, and are generally tricky to handle correctly.

Condition variables are meant to allow blocking until a condition
becomes true, with an undetermined number waiters and signalers,
and in many cases with a condition that is not simply "item
available".

Simple real world example:

    iter.gotReply.Wait(func() bool {
        iter.m.Lock()
        if iter.err == nil && iter.pendingDocs > 0 && iter.docData.Len() == 0 {
            iter.m.Unlock()
            return false
        }
        return true
    })

How to do this with a channel?

[rsc, 2011-01-05 19:34:30]

> Simple real world example:
>
>    iter.gotReply.Wait(func() bool {
>        iter.m.Lock()
>        if iter.err == nil && iter.pendingDocs > 0 && iter.docData.Len()
> == 0 {
>            iter.m.Unlock()
>            return false
>        }
>        return true
>    })
>
> How to do this with a channel?

I don't have any idea what this code does.
But that's the point.  The fact that all the locking
is in there makes me even more worried about
people using it correctly.

(Also you didn't show anything waking it up.)

I know that people can use channels correctly.
People tend not to be able to use condition
variables correctly.

Generality is not the only requirement of a good API.

Russ

[niemeyer, 2011-01-05 23:03:47]

> I don't have any idea what this code does.

It waits. ;-)

> (Also you didn't show anything waking it up.)

gotReply.Signal() or gotReply().Broadcast() is the way to poke it,
as usual for condition variables.

> Generality is not the only requirement of a good API.

Pointing out that the only thing I had in mind when developing
this API was generality is somewhat harsh, even more when you
haven't really provided any constructive advice I could use to
produce a better version.

I'll step down with this proposal and move that into a third
party package.

Thanks for your time.

[rsc, 2011-01-06 00:09:24]

The problem is that I don't know what problem
you're trying to solve.  You wrote:

> Condition variables are meant to allow blocking until a condition
> becomes true, with an undetermined number waiters and signalers,
> and in many cases with a condition that is not simply "item
> available".

They are undoubtedly a useful building block, but I don't
think that package sync should be building blocks.
It should be things that are easy to use correctly.

I thought that your condition in the example was supposed
to illustrate the tail of what you said, and I don't understand
how it does.  I thought your example was trying to show
a case where a channel was insufficient, and I didn't see
that it did that.

I think there probably needs to be a separate sync/subtle
package for the pieces that are obviously necessary but
hard to use correctly, like compare and swap, and maybe
also condition variables.

I'd definitely be happier with a condition variable that handled
the locking at the same time, like I believe pthread's cond_t does
or like the Rendez here:
http://swtch.com/plan9port/man/man3/lock.html
But it might equally be the case that the general one belongs
in sync/subtle.  I don't know.

I do know that when someone runs godoc sync, I only want
them to see the tools that we think people should reach for
every day.

Russ

[niemeyer, 2011-01-06 01:27:04]

> They are undoubtedly a useful building block, but I don't
> think that package sync should be building blocks.
> It should be things that are easy to use correctly.

That's useful information for an outsider. Thanks. At
least I have an idea of what's the scope.

> I thought that your condition in the example was supposed
> to illustrate the tail of what you said, and I don't understand
> how it does.  I thought your example was trying to show
> a case where a channel was insufficient, and I didn't see
> that it did that.

If you understand pthread's condition variables, the only
distinction is that rather than doing something like, say:

pthread_mutex_lock(&cond_var);
while (!condition()) {
    pthread_cond_wait(&cond_var, &some_mutex);
}

One does

cond_var.Wait(func() bool {
    some_mutex.Lock()
    if condition() { return true }
    some_mutex.UnlocK()
    return true
})

pthread_cond_signal() and pthread_cond_broadcast() have exactly the
same semantics.

> I'd definitely be happier with a condition variable that handled
> the locking at the same time, like I believe pthread's cond_t does
> or like the Rendez here:
> http://swtch.com/plan9port/man/man3/lock.html
(...)
> I do know that when someone runs godoc sync, I only want
> them to see the tools that we think people should reach for
> every day.

Ok, so are you interested in having this in a subpackage if I change it
to behave like, say:

   cond.Wait(mutex)
   cond.RWait(rwmutex)

?

[niemeyer, 2011-01-06 01:45:58]

Actually, disregard cond.RWait(rwmutex) please.  We need a mutual exclusion
either way to safely access internal structures, so we may as well use an
external one and save allocation and handling of one lock.

[niemeyer, 2011-01-06 01:53:32]

Spoke too soon. Signal() and Broadcast() would need the internal mutex, so
RWait() won't really present additional waste.

The proposed interface is this:

// Wait on the condition, and unlocks/locks the mutex as necessary.
cond.Wait(&mutex)

// Wait on the condition, and RLock/RUnlock the rwmutex as necessary.
cond.RWait(&rwmutex)

// Wake up the oldest sleeping goroutine.
cond.Signal()

// Wake up all goroutines.
cond.Broadcast()

[rsc, 2011-01-06 03:29:45]

Is there a reason not to make Wait and Signal
direct methods of the locks?

Russ

[niemeyer, 2011-01-06 12:04:25]

> Is there a reason not to make Wait and Signal
> direct methods of the locks?

It would certainly look and feel nice, but all mutexes would
have to carry at least one extra pointer to hold the conditional
struct. Is it worth the extra memory usage everywhere else?

One option to avoid that memory waste is to go into the opposite
direction, and turn Cond and RWCond into mutexes with Lock() etc.

[niemeyer, 2011-01-06 13:20:03]

FWIW, I can possibly reduce the amount of memory used for the
structure considerably basing the logic on semaphores. Still,
it'd be greater than zero of course, and it might change the
FIFO semantics depending on how semaphores behave in that
regard.

[niemeyer, 2011-01-06 14:38:35]

I've just uploaded a *much* simpler version based on semaphores
and cas, and turning the condition variable into a RW lock by itself.

I haven't tested this version yet, but I believe it should be
(close to :-) working correctly.

What do you think?

[rsc, 2011-01-06 15:25:08]

That looks a lot better.

I don't think you want the RWMutex to be embedded,
because it means each mutex can support only one
condition, and often you have multiple conditions
associated with a single locked data structure
(in the case of a queue, full and empty).

So it should probably be a pointer to an RWMutex,
not embedded.

type RWCond {
    M *RWMutex
    ...
}

type Cond {
    M *Mutex
    ...
}

or, if we were willing to give up RWait, it could be

type Locker interface {
    Lock()
    Unlock()
}

type Cond {
    M Locker
    ...
}

Even better, you could imagine doing

type rlocker RWMutex

func (r *rlocker) Lock() {
    (*RWMutex)(r).RLock()
}

func (r *rlocker) Unlock() {
    (*RWMutex)(r).RUnlock()
}

func (m *RWMutex) RLocker() Locker {
    return (*rlocker)(m)
}

[niemeyer, 2011-01-06 17:16:20]

> That looks a lot better.

Thanks.

> I don't think you want the RWMutex to be embedded,
> because it means each mutex can support only one
> condition, and often you have multiple conditions
> associated with a single locked data structure
> (in the case of a queue, full and empty).

Hmm, indeed.

> or, if we were willing to give up RWait, it could be
> 
> type Locker interface {
(...)
> func (m *RWMutex) RLocker() Locker {

These look pretty good. I imagined using an interface, but
was missing a good integration point with RWMutex. Will
refactor in that direction later today.

[niemeyer, 2011-01-06 21:16:40]

Refactored as discussed, races fixed, tests added.

PTAL

[niemeyer, 2011-01-12 16:50:02]

Last night I finally ported my in-development DB library to work
with this new interface, and the code actually looks nicer and
cleaner than before, so thanks for the interaction on it.

Do you think this is ready to go in as it is or would you like
me to polish any other edges?

[rog, 2011-01-12 17:26:58]

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond.go
File src/pkg/sync/cond.go (right):

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond.go#newcode11
src/pkg/sync/cond.go:11: M    Locker
just one thought: is it actually useful to have this as a public field? i don't
think it's possible to modify it safely in the presence of other goroutines
acting on the Cond. perhaps Cond could define Lock and Unlock methods which
operate on the mutex provided to NewCond.

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond.go#newcode43
src/pkg/sync/cond.go:43: // There is no guarantee about which goroutine will be
awaken first.
s/awaken/awoken/

[niemeyer, 2011-01-12 17:43:29]

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond.go
File src/pkg/sync/cond.go (right):

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond.go#newcode11
src/pkg/sync/cond.go:11: M    Locker
> just one thought: is it actually useful to have this as a public field? i
don't
> think it's possible to modify it safely in the presence of other goroutines

It is useful because it enables using the condition variable in a struct as:

type T struct {
  m sync.Mutex
  c sync.Cond
}

Without it being public, a separate allocation of the cond variable is enforced.

> acting on the Cond. perhaps Cond could define Lock and Unlock methods which
> operate on the mutex provided to NewCond.

That's doable with cond.M.Lock() / cond.M.Unlock()

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond.go#newcode43
src/pkg/sync/cond.go:43: // There is no guarantee about which goroutine will be
awaken first.
> s/awaken/awoken/

Looks correct to me: http://www.thefreedictionary.com/awaken

[rog, 2011-01-12 18:03:11]

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond.go
File src/pkg/sync/cond.go (right):

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond.go#newcode11
src/pkg/sync/cond.go:11: M    Locker
On 2011/01/12 17:43:29, niemeyer wrote:
> > just one thought: is it actually useful to have this as a public field? i
> don't
> > think it's possible to modify it safely in the presence of other goroutines
> 
> It is useful because it enables using the condition variable in a struct as:
> 
> type T struct {
>   m sync.Mutex
>   c sync.Cond
> }
> 
> Without it being public, a separate allocation of the cond variable is
enforced.

good point. an alternative might be to make M private and
provide a method
  func (c *Cond) InitLocker(m Locker)
which would fail if called twice.

that way at least there's no public field that looks as if it's designed to be
played with.

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond.go#newcode43
src/pkg/sync/cond.go:43: // There is no guarantee about which goroutine will be
awaken first.
On 2011/01/12 17:43:29, niemeyer wrote:
> > s/awaken/awoken/
> 
> Looks correct to me: http://www.thefreedictionary.com/awaken

woken, awoken or awakened would all be ok.
you could also say "which goroutine it will awaken" which is perhaps slightly
preferable. http://www.dailywritingtips.com/awoken-or-awakened/

[rsc, 2011-01-12 18:26:33]

A bunch of comments, mainly about comments.
The code is very nice and clean.
The test has a few races.

I am tempted to suggest Sleep, Wakeup, and WakeupAll
instead of the much less suggestive Signal and Broadcast,
but I am not suggesting them yet.  I want to think more.

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/Makefile
File src/pkg/sync/Makefile (right):

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/Makefile#newcode12
src/pkg/sync/Makefile:12: cond.go\
sort list

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond.go
File src/pkg/sync/cond.go (right):

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond.go#newcode1
src/pkg/sync/cond.go:1: package sync
copyright notice; see http://golang.org/doc/contribute.html#copyright

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond.go#newcode3
src/pkg/sync/cond.go:3: 
1 blank line is fine.

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond.go#newcode6
src/pkg/sync/cond.go:6: 
same here

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond.go#newcode7
src/pkg/sync/cond.go:7: // Cond enables waiting reliably for an arbitrary
condition to become
// Cond implements a condition variable, a rendezvous point
// for goroutines waiting for or announcing the occurrence
// of an event.
//
// Each Cond has an associated locker M (often a *Mutex or *RWMutex),
// which must be held when changing the condition and
// when calling the Wait method.
//
// Wait atomically unlocks M and suspends execution
// of the calling goroutine.  After later resuming execution,
// Wait will lock M before returning to the caller.
//
// Signal wakes one goroutine if there are any waiting on Cond.
// It returns 1 if it woke a goroutine, 0 otherwise.
//
// Broadcast wakes all goroutines waiting on Cond.
// It returns the number of goroutines awakened.

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond.go#newcode13
src/pkg/sync/cond.go:13: n    uint32
s/n/nwait/

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond.go#newcode17
src/pkg/sync/cond.go:17: // NewCond returns a new Cond variable with M
initialized to m.  The Mutex and
// NewCond returns a new Cond with locker m.

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond.go#newcode24
src/pkg/sync/cond.go:24: // Wait blocks the calling goroutine and waits until it
is awaken by
// Wait atomically unlocks c.M and suspends execution
// of the calling goroutine.  After later resuming execution,
// Wait locks c.M before returning.
//
// Because M is not locked when Wait first resumes, the caller
// typically cannot assume that the condition is true when
// Wait returns.  Instead, the caller should Wait in a loop:
//
//	c.M.Lock()
//	for !condition() {
//		c.Wait()
//	}
//	... make use of condition ...
//	c.M.Unlock()
//

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond.go#newcode42
src/pkg/sync/cond.go:42: // Signal awakes one goroutine currently waiting on the
condition variable.
// Signal wakes one goroutine waiting on c, if there are any.
// It returns 1 if it woke a goroutine, 0 otherwise.
//
// It is allowed but not required for the caller to hold c.M
// during the call.

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond.go#newcode44
src/pkg/sync/cond.go:44: func (c *Cond) Signal() {
return 1 or 0

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond.go#newcode53
src/pkg/sync/cond.go:53: // Broadcast awakes all goroutines currently waiting on
the condition
// Broadcast wakes all goroutines waiting on c.
// It returns the number of goroutines awakened.
//
// It is allowed but not required for the caller to hold c.M
// during the call.

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond.go#newcode55
src/pkg/sync/cond.go:55: func (c *Cond) Broadcast() {
add return value

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond.go#newcode63
src/pkg/sync/cond.go:63: c.sema = nil // Prevent races.
Needs more of a comment.

// We just issued n wakeups via the semaphore s.
// To ensure that they wake up the existing waiters
// and not waiters that arrive after Broadcast returns,
// clear c.sema.  The next operation will allocate
// a new one.

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond_test.go
File src/pkg/sync/cond_test.go (right):

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond_test.go#ne...
src/pkg/sync/cond_test.go:1: package sync_test
copyright

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond_test.go#ne...
src/pkg/sync/cond_test.go:3: 
single blank line

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond_test.go#ne...
src/pkg/sync/cond_test.go:9: 
single blank line

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond_test.go#ne...
src/pkg/sync/cond_test.go:15: awaken := make(chan bool, n)
s/awaken/awake/ - it's a description, like running

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond_test.go#ne...
src/pkg/sync/cond_test.go:16: for i := 0; i != n; i++ {
i < n is more idiomatic in the go code base.
applies throughout this file

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond_test.go#ne...
src/pkg/sync/cond_test.go:17: go func(g int) {
parameter is not used?

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond_test.go#ne...
src/pkg/sync/cond_test.go:30: t.Fatal("Goroutine should be sleeping")
lowercase: goroutine not asleep

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond_test.go#ne...
src/pkg/sync/cond_test.go:32: c.Signal()
This is not guaranteed to find someone to wake.
Should be

m.Lock()
c.Signal()
m.Unlock()

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond_test.go#ne...
src/pkg/sync/cond_test.go:33: <-awaken // Will panic unless at least one is
awaken.
// will deadlock if no goroutine wakes up

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond_test.go#ne...
src/pkg/sync/cond_test.go:35: t.Fatal("Too many goroutines awaken")
too many goroutines awake

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond_test.go#ne...
src/pkg/sync/cond_test.go:63: if i == n-1 {
need to lock m while setting exit = true.

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond_test.go#ne...
src/pkg/sync/cond_test.go:67: t.Fatal("Goroutine should be sleeping")
s/G/g/

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond_test.go#ne...
src/pkg/sync/cond_test.go:69: c.Broadcast()
need to lock m around call to Broadcast
in order to make sure that all the goroutines 
have successfully executed Wait

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond_test.go#ne...
src/pkg/sync/cond_test.go:70: seen := make(map[int]bool, n)
seen := make([]bool, n)

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond_test.go#ne...
src/pkg/sync/cond_test.go:74: t.Fatal("Same goroutine awaken twice")
goroutine woke up twice

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/cond_test.go#ne...
src/pkg/sync/cond_test.go:80: t.Fatal("Goroutine should have exited")
goroutine did not exit

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/mutex.go
File src/pkg/sync/mutex.go (right):

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/mutex.go#newcode24
src/pkg/sync/mutex.go:24: // Interface implemented by objects which may be
locked and unlocked.
// A Locker represents an object that can be locked and unlocked.

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/mutex.go#newcode25
src/pkg/sync/mutex.go:25: // Both Mutex and RWMutex implement this interface. 
See RWMutex.RLocker()
drop this comment

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/rwmutex.go
File src/pkg/sync/rwmutex.go (right):

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/rwmutex.go#newc...
src/pkg/sync/rwmutex.go:78: // RLock() and RUnlock() in rw.
// RLocker returns a Locker interface that implements
// the Lock and Unlock methods by calling rw.RLock and rw.RUnlock.

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/rwmutex_test.go
File src/pkg/sync/rwmutex_test.go (right):

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/rwmutex_test.go...
src/pkg/sync/rwmutex_test.go:124: for i := 0; i != n; i++ {
i < n

http://codereview.appspot.com/3775048/diff/34001/src/pkg/sync/rwmutex_test.go...
src/pkg/sync/rwmutex_test.go:132: for i := 0; i != n; i++ {
i < n

[rsc, 2011-01-12 18:28:04]

> good point. an alternative might be to make M private and
> provide a method
>  func (c *Cond) InitLocker(m Locker)
> which would fail if called twice.

I really like being able to use composite literals.

This would be func (c *Cond) Init(m Locker)
if anything, but I'd prefer to keep it as is.

Russ

[niemeyer, 2011-01-12 19:22:27]

> A bunch of comments, mainly about comments.
> The code is very nice and clean.

Thanks, and thanks for the great comments. Will
try to keep the style issues in mind for the future.
Also tested the return values for Signal and Broadcast.

> The test has a few races.

I noticed the problem, and pushed the fix a few minutes
before your review. Sorry about that.

It's ready for another round.

[niemeyer, 2011-02-01 09:47:25]

Updated tests after recent changes to non-blocking channel sends.

[niemeyer, 2011-02-04 00:01:14]

Patch wasn't applying cleanly anymore.  Had to download the patch,
clean it up, and push the changes again.  Not sure why the patch
set description changed above, but it's still the same patch.

[rsc, 2011-02-11 21:30:31]

LGTM

I think this turned out nice. 
Adding r to reviewer list.

[r, 2011-02-15 23:42:01]

http://codereview.appspot.com/3775048/diff/62001/src/pkg/sync/cond.go
File src/pkg/sync/cond.go (right):

http://codereview.appspot.com/3775048/diff/62001/src/pkg/sync/cond.go#newcode12
src/pkg/sync/cond.go:12: // Each Cond has an associated locker M (often a *Mutex
or *RWMutex),
s/locker/Locker/ ?

http://codereview.appspot.com/3775048/diff/62001/src/pkg/sync/cond.go#newcode24
src/pkg/sync/cond.go:24: // It returns the number of goroutines awakened.
why do you describe all the methods here?

http://codereview.appspot.com/3775048/diff/62001/src/pkg/sync/cond.go#newcode28
src/pkg/sync/cond.go:28: n    int
comments please. M and m are unrelated and confusing.
and why is a Locker called 'M'? 'L' makes more sense without context.

http://codereview.appspot.com/3775048/diff/62001/src/pkg/sync/cond.go#newcode65
src/pkg/sync/cond.go:65: // Signal wakes one goroutine waiting on c, if there
are any.
s/are any/is one/ or /is any/ (the subject of the verb is 'one goroutine'.

http://codereview.appspot.com/3775048/diff/62001/src/pkg/sync/cond.go#newcode70
src/pkg/sync/cond.go:70: func (c *Cond) Signal() int {
why not a boolean?

[rsc, 2011-02-16 04:53:40]

>
http://codereview.appspot.com/3775048/diff/62001/src/pkg/sync/cond.go#newcode70
> src/pkg/sync/cond.go:70: func (c *Cond) Signal() int {
> why not a boolean?

for consistency with Broadcast.
both return the number of goroutines awakened.

[r, 2011-02-16 07:20:55]

On Tue, Feb 15, 2011 at 8:53 PM, Russ Cox <rsc@golang.org> wrote:
>>
http://codereview.appspot.com/3775048/diff/62001/src/pkg/sync/cond.go#newcode70
>> src/pkg/sync/cond.go:70: func (c *Cond) Signal() int {
>> why not a boolean?
>
> for consistency with Broadcast.
> both return the number of goroutines awakened.

I don't think that's compelling.  I need to check against 0 or 1 to
use it; no other value is valid.  Signal doesn't sound like an
int-valued function.

if c.Signal() == 1 { ... } isn't a priori obvious in intent.

-rob

[niemeyer, 2011-02-16 07:32:57]

All sorted. PTAL

[r, 2011-02-16 16:19:24]

this seems good to me modulo two comment tweaks.

leaving for rsc.

http://codereview.appspot.com/3775048/diff/70001/src/pkg/sync/cond.go
File src/pkg/sync/cond.go (right):

http://codereview.appspot.com/3775048/diff/70001/src/pkg/sync/cond.go#newcode56
src/pkg/sync/cond.go:56: // It returns true if it woke a goroutine up.
woke up a goroutine
or even better
woke a goroutine
the latter is consistent with the explanation for Broadcast

[rsc, 2011-02-16 16:22:49]

> I don't think that's compelling.  I need to check against 0 or 1 to
> use it; no other value is valid.  Signal doesn't sound like an
> int-valued function.
>
> if c.Signal() == 1 { ... } isn't a priori obvious in intent.

I asked for the return values because I had used them
in the Rendez data type I added to plan9port.  There the
consistent return values made more sense because
Signal and Broadcast were called the more obviously
related names rwakeup and rwakeupall.
http://swtch.com/plan9port/man/man3/lock.html

That said, I looked through the code I wrote using them
and not one call site uses the return value.  It sounds like
we should just get rid of it.  Using it would be racy anyway.

Russ

[r, 2011-02-16 16:24:08]

getting rid of it is ok too.

it's in your court. i agree this turned out well; leaving for you to finalize.

-rob

[rsc, 2011-02-16 16:34:29]

gustavo, if you agree, go ahead and remove the return values.

[niemeyer, 2011-02-16 17:21:53]

> gustavo, if you agree, go ahead and remove the return values.

I don't have a use case for them either right now, and
if it turns out to be a good idea, it's always easier
to add them than to remove them later.

I've dropped the returns. PTAL

[rsc, 2011-02-16 19:11:12]

*** Submitted as d3bc87b547a4 ***

sync: add Cond

R=rsc, rog, r
CC=golang-dev
http://codereview.appspot.com/3775048

Committer: Russ Cox <rsc@golang.org>