|
|
Created:
13 years, 4 months ago by niemeyer Modified:
13 years, 2 months ago Reviewers:
CC:
rsc, rog, r, golang-dev Visibility:
Public. |
Descriptionsync: condition variable support.
This change introduces a new condition variable implementation which helps solving some well known synchronization problems in a simple way.
Patch Set 1 #Patch Set 2 : code review 3775048: sync: Proposal for condition variable support. #Patch Set 3 : code review 3775048: sync: Proposal for condition variable support. #Patch Set 4 : code review 3775048: sync: Proposal for condition variable support. #Patch Set 5 : code review 3775048: sync: Proposal for condition variable support. #Patch Set 6 : code review 3775048: sync: Proposal for condition variable support. #Patch Set 7 : code review 3775048: sync: Proposal for condition variable support. #Patch Set 8 : code review 3775048: sync: Proposal for condition variable support. #Patch Set 9 : code review 3775048: sync: Proposal for condition variable support. #
Total comments: 40
Patch Set 10 : code review 3775048: sync: Proposal for condition variable support. #Patch Set 11 : code review 3775048: sync: Proposal for condition variable support. #Patch Set 12 : code review 3775048: sync: Proposal for condition variable support. #Patch Set 13 : diff -r f37c651d8774 https://go.googlecode.com/hg/ #Patch Set 14 : diff -r f37c651d8774 https://go.googlecode.com/hg/ #
Total comments: 5
Patch Set 15 : diff -r 91202941cf4c https://go.googlecode.com/hg/ #
Total comments: 1
Patch Set 16 : diff -r 91202941cf4c https://go.googlecode.com/hg/ #Patch Set 17 : diff -r 91202941cf4c https://go.googlecode.com/hg/ #Patch Set 18 : diff -r 91202941cf4c https://go.googlecode.com/hg/ #
MessagesTotal messages: 36
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change.
Sign in to reply to this message.
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
Sign in to reply to this message.
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?
Sign in to reply to this message.
> 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
Sign in to reply to this message.
> 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.
Sign in to reply to this message.
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
Sign in to reply to this message.
> 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) ?
Sign in to reply to this message.
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.
Sign in to reply to this message.
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()
Sign in to reply to this message.
Is there a reason not to make Wait and Signal direct methods of the locks? Russ
Sign in to reply to this message.
> 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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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?
Sign in to reply to this message.
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) }
Sign in to reply to this message.
> 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.
Sign in to reply to this message.
Refactored as discussed, races fixed, tests added. PTAL
Sign in to reply to this message.
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?
Sign in to reply to this message.
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/
Sign in to reply to this message.
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
Sign in to reply to this message.
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/
Sign in to reply to this message.
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
Sign in to reply to this message.
> 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
Sign in to reply to this message.
> 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.
Sign in to reply to this message.
Updated tests after recent changes to non-blocking channel sends.
Sign in to reply to this message.
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.
Sign in to reply to this message.
LGTM I think this turned out nice. Adding r to reviewer list.
Sign in to reply to this message.
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?
Sign in to reply to this message.
> 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.
Sign in to reply to this message.
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
Sign in to reply to this message.
All sorted. PTAL
Sign in to reply to this message.
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
Sign in to reply to this message.
> 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
Sign in to reply to this message.
getting rid of it is ok too. it's in your court. i agree this turned out well; leaving for you to finalize. -rob
Sign in to reply to this message.
gustavo, if you agree, go ahead and remove the return values.
Sign in to reply to this message.
> 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
Sign in to reply to this message.
*** Submitted as d3bc87b547a4 *** sync: add Cond R=rsc, rog, r CC=golang-dev http://codereview.appspot.com/3775048 Committer: Russ Cox <rsc@golang.org>
Sign in to reply to this message.
|