Issue 57700044: voyeur package

Can't Edit
Can't Publish+Mail
Start Review

Created:
11 years, 1 month ago by natefinch

Modified:
11 years, 1 month ago

Reviewers:
dimitern, mp+205379, rog

Visibility:
Public.

Description

voyeur 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 #

Created: 11 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+338 lines, -0 lines)			Patch
A	[revision details]	View	1 2	1 chunk	+2 lines, -0 lines	0 comments	Download
A	utils/voyeur/value.go	View	1 2	1 chunk	+121 lines, -0 lines	0 comments	Download
A	utils/voyeur/value_test.go	View	1	1 chunk	+215 lines, -0 lines	0 comments	Download

Messages

Total messages: 9

Expand All Messages | Collapse All Messages

rog

Looks great, thanks (well I would say that :-]), but a few more tests would ...

11 years, 1 month ago (2014-02-07 15:15:17 UTC) #2

natefinch

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 ...

11 years, 1 month ago (2014-02-07 20:05:20 UTC) #3

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.

dimitern

Looking good, apart from the unusual watcher implementation and a icky name :) Some thoughts ...

11 years, 1 month ago (2014-02-10 09:15:30 UTC) #4

natefinch

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): ...

11 years, 1 month ago (2014-02-10 12:41:21 UTC) #5

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.

rog

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#newcode37 utils/voyeur/value.go:37: v.val = val On 2014/02/10 09:15:30, dimitern wrote: > ...

11 years, 1 month ago (2014-02-10 12:52:12 UTC) #6

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
}

rog

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#newcode58 utils/voyeur/value.go:58: if v.closed { return v.val, !v.closed https://codereview.appspot.com/57700044/diff/20001/utils/voyeur/value.go#newcode77 utils/voyeur/value.go:77: // ...

11 years, 1 month ago (2014-02-10 13:32:14 UTC) #7

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
}

rog

11 years, 1 month ago (2014-02-13 17:32:00 UTC) #9

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.

Expand All Messages | Collapse All Messages