Issue 57700044: voyeur package
Descriptionvoyeur package
The voyeur package is intended for use in concurrent environments where multiple people need to read and write a single value, and watchers want to be notified when changes occur to the value.
https://code.launchpad.net/~natefinch/juju-core/032-voyeur/+merge/205379
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 12
Patch Set 2 : voyeur package #
Total comments: 30
Patch Set 3 : voyeur package #
MessagesTotal messages: 9
Please take a look.
Sign in to reply to this message.
Looks great, thanks (well I would say that :-]), but a few more tests would be good. https://codereview.appspot.com/57700044/diff/1/utils/voyeur/value.go File utils/voyeur/value.go (right): https://codereview.appspot.com/57700044/diff/1/utils/voyeur/value.go#newcode59 utils/voyeur/value.go:59: return nil, false i wonder if this should return v.val anyway. i can think of arguments both ways. anyway, if we don't change it to return v.val, the docs should say that the returned value is nil if it's closed. https://codereview.appspot.com/57700044/diff/1/utils/voyeur/value.go#newcode79 utils/voyeur/value.go:79: // Next reports whether the value or the Watcher itself has been closed. The final sentence isn't quite accurate (it would seem to imply that it returns true when closed). Perhaps: // Next returns false if the value or the Watcher itself have // been closed. (I can't think of a nice way of using the "reports whether" idiom) https://codereview.appspot.com/57700044/diff/1/utils/voyeur/value.go#newcode96 utils/voyeur/value.go:96: // and then reacquires the lock, thus avoiding a deadlock. // Note that Wait releases the lock, waits for a // condition change, and then reacquires the lock. ? This is standard idiomatic behaviour if you're familiar with the sync primitives. It's not really "magic sauce" :-) https://codereview.appspot.com/57700044/diff/1/utils/voyeur/value.go#newcode110 utils/voyeur/value.go:110: // Value returns the last value that was retrieved from the watched Value. s/./by Next./ ? https://codereview.appspot.com/57700044/diff/1/utils/voyeur/value_test.go File utils/voyeur/value_test.go (right): https://codereview.appspot.com/57700044/diff/1/utils/voyeur/value_test.go#new... utils/voyeur/value_test.go:34: <-ch I'd prefer if this was a send operation, because then the direction mirrors the data flow. https://codereview.appspot.com/57700044/diff/1/utils/voyeur/value_test.go#new... utils/voyeur/value_test.go:90: v.Set(s) We should probably have tests that - we can call Set several times without the receiver needing to call Next. - that we can have several independent receivers. - that receivers can be closed.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/57700044/diff/1/utils/voyeur/value.go File utils/voyeur/value.go (right): https://codereview.appspot.com/57700044/diff/1/utils/voyeur/value.go#newcode59 utils/voyeur/value.go:59: return nil, false On 2014/02/07 15:15:17, rog wrote: > i wonder if this should return v.val anyway. > > i can think of arguments both ways. > > anyway, if we don't change it to return v.val, the docs should say that the > returned value is nil if it's closed. Probably no big deal to return val regardless, fixed. https://codereview.appspot.com/57700044/diff/1/utils/voyeur/value.go#newcode79 utils/voyeur/value.go:79: // Next reports whether the value or the Watcher itself has been closed. On 2014/02/07 15:15:17, rog wrote: > The final sentence isn't quite accurate (it would seem to imply that it returns > true when closed). > > Perhaps: > > // Next returns false if the value or the Watcher itself have > // been closed. > > (I can't think of a nice way of using the "reports whether" idiom) Good point, you're right about it implying it returns true. Fixed. https://codereview.appspot.com/57700044/diff/1/utils/voyeur/value.go#newcode96 utils/voyeur/value.go:96: // and then reacquires the lock, thus avoiding a deadlock. On 2014/02/07 15:15:17, rog wrote: > // Note that Wait releases the lock, waits for a > // condition change, and then reacquires the lock. > > ? > > This is standard idiomatic behaviour if you're familiar > with the sync primitives. It's not really "magic sauce" :-) Well, yes, perhaps magic sauce is not a good comment. However, mentioning that it actually doesn't cause a deadlock, even though it looks like it should is probably a good thing (not everyone is as conversant with the sync package as you :) Fixed. https://codereview.appspot.com/57700044/diff/1/utils/voyeur/value.go#newcode110 utils/voyeur/value.go:110: // Value returns the last value that was retrieved from the watched Value. On 2014/02/07 15:15:17, rog wrote: > s/./by Next./ > > ? Done. https://codereview.appspot.com/57700044/diff/1/utils/voyeur/value_test.go File utils/voyeur/value_test.go (right): https://codereview.appspot.com/57700044/diff/1/utils/voyeur/value_test.go#new... utils/voyeur/value_test.go:34: <-ch On 2014/02/07 15:15:17, rog wrote: > I'd prefer if this was a send operation, because > then the direction mirrors the data flow. Good point. Done. https://codereview.appspot.com/57700044/diff/1/utils/voyeur/value_test.go#new... utils/voyeur/value_test.go:90: v.Set(s) On 2014/02/07 15:15:17, rog wrote: > We should probably have tests that > - we can call Set several times without the > receiver needing to call Next. > > - that we can have several independent receivers. > > - that receivers can be closed. Ahh yeah, good point. I meant to add some more tests, will do that now.
Sign in to reply to this message.
Looking good, apart from the unusual watcher implementation and a icky name :) Some thoughts below. https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go File utils/voyeur/value.go (right): https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go#newc... utils/voyeur/value.go:1: // Copyright 2012, 2013 Canonical Ltd. s/2012, 2013/2014/ https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go#newc... utils/voyeur/value.go:6: package voyeur The name makes me cringe a little. I know it's intended to be funny, but would you consider renaming it to "observer" or something like that? https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go#newc... utils/voyeur/value.go:37: v.val = val My spider sense is tingling for: defer v.mu.Unlock() (here and everywhere relevant). https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go#newc... utils/voyeur/value.go:43: // Close closes the Value, unblocking any outstanding watchers. Close always Why Close? Why not Release or something. Close doesn't make it immediately obvious what's being done. https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go#newc... utils/voyeur/value.go:77: // Next blocks until there is a new value to be retrieved from the value that is Shouldn't the watcher have an internal loop spawn separately and a Changes() chan interface{} method instead or Next() and Value()? https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go#newc... utils/voyeur/value.go:85: // We should never go around this loop more than twice. How is this evident? Perhaps either remove this comment or expand it a bit. https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go#newc... utils/voyeur/value.go:96: // Wait releases the lock until triggered and then reacquires the lock, s/the lock/the conditional/ ? https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value_test.go File utils/voyeur/value_test.go (right): https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value_test.go... utils/voyeur/value_test.go:1: // Copyright 2012, 2013 Canonical Ltd. s/2012, 2013/2014/ https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value_test.go... utils/voyeur/value_test.go:27: // The channel is not necessary for normal use of the watcher. But it should be - it's more consistent with the other watchers. More over, we could use the statetesting package to test the watcher (perhaps then we could move it out of state and in some more general testing package). https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value_test.go... utils/voyeur/value_test.go:80: vals := []string{"one", "two", "three"} This block (from vals := .. to v := NewValue(nil) is repeated several times. Factor it out in a helper, please. https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value_test.go... utils/voyeur/value_test.go:214: d
Sign in to reply to this message.
I'll look into making a couple of the modifications you suggested. https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go File utils/voyeur/value.go (right): https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go#newc... utils/voyeur/value.go:6: package voyeur On 2014/02/10 09:15:30, dimitern wrote: > The name makes me cringe a little. I know it's intended to be funny, but would > you consider renaming it to "observer" or something like that? I honestly didn't intend it to be funny, I just wanted something unique that gave you an idea of what it does. Observer is a fairly well-known pattern (that I am not particularly fond of), so I'd prefer not to use that, since it could be confusing for some people. https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go#newc... utils/voyeur/value.go:37: v.val = val On 2014/02/10 09:15:30, dimitern wrote: > My spider sense is tingling for: > defer v.mu.Unlock() (here and everywhere relevant). Welcome to a world with no exceptions :) It's perfectly safe to just wait until the end of the function and call it explicitly, but I take your meaning. Probably calling defer is the right call (so if we add code that exits the function in the middle, it'll still unlock). https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go#newc... utils/voyeur/value.go:43: // Close closes the Value, unblocking any outstanding watchers. Close always On 2014/02/10 09:15:30, dimitern wrote: > Why Close? Why not Release or something. Close doesn't make it immediately > obvious what's being done. We debated the name for this. Close is the standard "we're done with this object" method. I don't know that Release or End or Stop would be any more obvious... honestly, Close seems more intuitive to me, because I don't have to wonder if this is some special implementation that does something different than a normal Close function. https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go#newc... utils/voyeur/value.go:77: // Next blocks until there is a new value to be retrieved from the value that is On 2014/02/10 09:15:30, dimitern wrote: > Shouldn't the watcher have an internal loop spawn separately and a Changes() > chan interface{} method instead or Next() and Value()? That's probably a good idea, especially since right now, the only place we use it we do exactly that - turn the output into a channel. https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go#newc... utils/voyeur/value.go:85: // We should never go around this loop more than twice. On 2014/02/10 09:15:30, dimitern wrote: > How is this evident? Perhaps either remove this comment or expand it a bit. We can expand the comment. The reason is that if we get to the end of the for loop, we wait on the conditional, and the only way the conditional can fire is if the value closes, the watcher closes, or the version changes. https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go#newc... utils/voyeur/value.go:96: // Wait releases the lock until triggered and then reacquires the lock, On 2014/02/10 09:15:30, dimitern wrote: > s/the lock/the conditional/ ? The Wait() function actually releases the lock that was passed into the conditional while waiting and then reacquires it before returning. It's important to state that it's the lock, since we lock the lock at the top of the function, so if you don't know how Wait() works, it looks like a deadlock (which is what I thought when I read Roger's implementation, which is why I added the note about how Wait works). https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value_test.go File utils/voyeur/value_test.go (right): https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value_test.go... utils/voyeur/value_test.go:1: // Copyright 2012, 2013 Canonical Ltd. On 2014/02/10 09:15:30, dimitern wrote: > s/2012, 2013/2014/ I wonder if we even really need a date here. Can we not just say Copyright Canonical Ltd and never have to worry about updating the date again? https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value_test.go... utils/voyeur/value_test.go:27: // The channel is not necessary for normal use of the watcher. On 2014/02/10 09:15:30, dimitern wrote: > But it should be - it's more consistent with the other watchers. More over, we > could use the statetesting package to test the watcher (perhaps then we could > move it out of state and in some more general testing package). This particular channel doesn't transmit any data or interact with the watcher at all. It just lets the two goroutines run in lock step, which is necessary to have repeatable tests... otherwise we're at the mercy of the scheduler, and it's not very consistent. https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value_test.go... utils/voyeur/value_test.go:80: vals := []string{"one", "two", "three"} On 2014/02/10 09:15:30, dimitern wrote: > This block (from vals := .. to v := NewValue(nil) is repeated several times. > Factor it out in a helper, please. It's just three lines of code, I don't really want to put it into a helper because I think it would make the code less readable. However, I could remove the comment from all but the first instance, which would make it less verbose.
Sign in to reply to this message.
https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go File utils/voyeur/value.go (right): https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go#newc... utils/voyeur/value.go:37: v.val = val On 2014/02/10 09:15:30, dimitern wrote: > My spider sense is tingling for: > defer v.mu.Unlock() (here and everywhere relevant). This is the only place. It's deliberate that Broadcast is called from outside the lock in other places. https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go#newc... utils/voyeur/value.go:43: // Close closes the Value, unblocking any outstanding watchers. Close always On 2014/02/10 09:15:30, dimitern wrote: > Why Close? Why not Release or something. Close doesn't make it immediately > obvious what's being done. It's analogous to closing a channel. https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go#newc... utils/voyeur/value.go:77: // Next blocks until there is a new value to be retrieved from the value that is On 2014/02/10 09:15:30, dimitern wrote: > Shouldn't the watcher have an internal loop spawn separately and a Changes() > chan interface{} method instead or Next() and Value()? If that's needed, it can be easily layered on top of this simple lower-level interface. This is an extremely lightweight API intended to be used as a primitive for many kinds of operation. https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go#newc... utils/voyeur/value.go:85: // We should never go around this loop more than twice. On 2014/02/10 09:15:30, dimitern wrote: > How is this evident? Perhaps either remove this comment or expand it a bit. Yes, perhaps it could do with a bit of expansion, say: // We can go around this loop a maximum of two times, // because the only thing that can cause a Wait to // return is for the condition to be triggered, // which can only happen if the value is set (causing // the version to increment) or it is closed // causing the closed flag to be set. // Both these cases will cause Next to return. https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go#newc... utils/voyeur/value.go:96: // Wait releases the lock until triggered and then reacquires the lock, On 2014/02/10 09:15:30, dimitern wrote: > s/the lock/the conditional/ ? It really is the lock - it's the lock that the condition refers to. You can't lock a condition. https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value_test.go File utils/voyeur/value_test.go (right): https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value_test.go... utils/voyeur/value_test.go:4: package voyeur Our usual convention is to have external tests - is there any particular reason we shouldn't do that here? https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value_test.go... utils/voyeur/value_test.go:27: // The channel is not necessary for normal use of the watcher. On 2014/02/10 09:15:30, dimitern wrote: > But it should be - it's more consistent with the other watchers. More over, we > could use the statetesting package to test the watcher (perhaps then we could > move it out of state and in some more general testing package). The main point of the state/testing package is that it integrates with state - in particular, it calls StartSync. If you take that functionality out, then the code in state/testing/watcher.go is honestly trivial - it's just a select with a timeout. The role of the channel in this code is not to be the watcher notification channel, but to be the synchronisation mechanism - we'd need it even if the interface to the Value was through a channel. https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value_test.go... utils/voyeur/value_test.go:80: vals := []string{"one", "two", "three"} On 2014/02/10 09:15:30, dimitern wrote: > This block (from vals := .. to v := NewValue(nil) is repeated several times. > Factor it out in a helper, please. These three lines are logically independent, and each one is trivial. I don't think that factoring them out into a helper that bunches them together will help the readability of the code. On the other hand, perhaps there is a higher level function wanting to expose itself here. Something like: // valueSetter sets each of the given values in turn // on the given Value. It sends on the returned // channel after each call to Set. func valueSetter(v *voyeur.Value, vals ...string) chan struct{} { ch := make(chan struct{}) go func() { for _, s := range vals { v.Set(s) ch <- struct{}{} } v.Close() }() return ch } then TestWatch might look like: func (s *suite) TestWatcher(c *gc.C) { vals := []string{"one", "two", "three"} v := NewValue(nil) ch := valueSetter(v, vals...) w := v.Watch() etc }
Sign in to reply to this message.
https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go File utils/voyeur/value.go (right): https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go#newc... utils/voyeur/value.go:58: if v.closed { return v.val, !v.closed https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go#newc... utils/voyeur/value.go:77: // Next blocks until there is a new value to be retrieved from the value that is On 2014/02/10 12:52:13, rog wrote: > On 2014/02/10 09:15:30, dimitern wrote: > > Shouldn't the watcher have an internal loop spawn separately and a Changes() > > chan interface{} method instead or Next() and Value()? > > If that's needed, it can be easily layered on top of this simple lower-level > interface. This is an extremely lightweight API intended to be used as a > primitive for many kinds of operation. For the record, if we *did* want a channel-based interface in this package, I would suggest something like the following API. Note that it follows the NotifyWatcher pattern, rather than sending the latest value over the channel - this means that the watching code will always obtain the latest value, even if the value is changing rapidly and the receiving code is slow. I don't think that it's worth it - jujud/machine.go gets only 2 lines shorter using it. // Notify returns a Notifier that can be used to obtain a notification // on a channel when the value is set. The returned notifier must be // killed when abandoned to prevent memory leaks. func (v *Value) Notify() *Notifier { return &Notifier{ w: v.Watch(), out: make(chan struct{}), } } type Notifier struct { tomb tomb.Tomb w *Watcher out chan struct{} } func (n *Notifier) loop() { defer n.tomb.Done() for n.w.Next() { select { case n.out <- struct{}{}: case <-tomb.Dying(): } } } // Changes returns a channel that sends a value when the value changes. // The value itself can be retrieved by calling the value's Get method. func (n *Notifier) Changes() <-chan struct{} { return n.out } // Kill stops the notifier but does not wait for it to finish. func (w *Notifier) Kill() { n.tomb.Kill(nil) n.w.Close() } // Wait waits for the notifier to finish. It always returns nil. func (n *Notifier) Wait() error { tomb.Wait() return nil }
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
On 2014/02/13 17:27:45, nate.finch wrote: > Please take a look. LGTM with a test that calls NewValue with a non-nil arg.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
