add presence.ChildrenWatcher

add presence.ChildrenWatcher

This mimics watcher.ChildrenWatcher, but expects that all children be
presence nodes; and notifies of child node Pinger life/death, rather
than mere existence of child nodes.

This is now much closer to the original proposal, but hopefully with less
apparent crack-related influences, and abandons the somewhat optimistic
(read: almost entirely composed of weapons-grade crack) attempt to make
the childLoop goroutines responsible for their own cleanup.

Original Cl at: https://codereview.appspot.com/6335057/ (CL changed for dull
prereq-related reasons).

https://code.launchpad.net/~fwereade/juju-core/watch-presence-children/+merge/112375

Requires: https://code.launchpad.net/~fwereade/juju-core/consolidate-content-watchers/+merge/112350

(do not edit description out of merge proposal)

[fwereade, 2012-06-27 15:01:12]

Please take a look.

[niemeyer, 2012-06-28 06:48:49]

https://codereview.appspot.com/6354045/diff/1/.lbox
File .lbox (left):

https://codereview.appspot.com/6354045/diff/1/.lbox#oldcode1
.lbox:1: propose -cr -for lp:juju-core
Oops. We still want this.

https://codereview.appspot.com/6354045/diff/1/state/presence/presence.go
File state/presence/presence.go (right):

https://codereview.appspot.com/6354045/diff/1/state/presence/presence.go#newc...
state/presence/presence.go:334: // Note that we never close w.updates: child
loops may still be attempting
I'd say simply:

// Don't close w.updates.

As a remark that it is intentional. The rest of the explanation is describing
the default behavior of channels.

https://codereview.appspot.com/6354045/diff/1/state/presence/presence.go#newc...
state/presence/presence.go:348: case ch, ok := <-cw.Changes():
All the logic below is quite clean. Thanks.

https://codereview.appspot.com/6354045/diff/1/state/presence/presence.go#newc...
state/presence/presence.go:396: go w.childLoop(key, path, aliveW)
I believe we don't have to run the loop when nodes are deleted. We've run the
loop once when we detected that the node was added, so we can let that old loop
that is still running notify about the deletion.

https://codereview.appspot.com/6354045/diff/1/state/presence/presence.go#newc...
state/presence/presence.go:401: // childLoop sends aliveChange events to
w.updates, in response to presence
Perhaps this might be called aliveLoop, or something else that gives a feeling
of what it does.

https://codereview.appspot.com/6354045/diff/1/state/presence/presence.go#newc...
state/presence/presence.go:425: stat, err := w.conn.Exists(path)
There's a race here. Consider this:

1. Node was removed.
2. liveness test on line 422 fails
3. Node is recreated
4. Exists on line above is called, returns stat != nil
5. A second loop is started because we got a new child added. Oops.

I think it's cleaner to have the deletions detected by the children watcher
being used to stop this loop from outside, similar to what you were doing
before. w.updates may continue to be fed from within this function only, though,
to avoid races and dups.

https://codereview.appspot.com/6354045/diff/1/state/presence/presence.go#newc...
state/presence/presence.go:433: }
Shouldn't it call AliveW again here? Please add a test before fixing it.

[fwereade, 2012-06-28 10:37:39]

On closer inspection it appears that a lot of this proposal is complete
nonsense. Sorry :(.

https://codereview.appspot.com/6354045/diff/1/.lbox
File .lbox (left):

https://codereview.appspot.com/6354045/diff/1/.lbox#oldcode1
.lbox:1: propose -cr -for lp:juju-core
On 2012/06/28 06:48:49, niemeyer wrote:
> Oops. We still want this.
I'll readd it -- but I couldn't make the propose work until I deleted it. I
expect I screwed something else up that I didn't notice.

https://codereview.appspot.com/6354045/diff/1/state/presence/presence.go
File state/presence/presence.go (right):

https://codereview.appspot.com/6354045/diff/1/state/presence/presence.go#newc...
state/presence/presence.go:334: // Note that we never close w.updates: child
loops may still be attempting
On 2012/06/28 06:48:49, niemeyer wrote:
> I'd say simply:
> 
> // Don't close w.updates.
> 
> As a remark that it is intentional. The rest of the explanation is describing
> the default behavior of channels.

SGTM.

https://codereview.appspot.com/6354045/diff/1/state/presence/presence.go#newc...
state/presence/presence.go:425: stat, err := w.conn.Exists(path)
On 2012/06/28 06:48:49, niemeyer wrote:
> There's a race here. Consider this:
> 
> 1. Node was removed.
> 2. liveness test on line 422 fails
> 3. Node is recreated
> 4. Exists on line above is called, returns stat != nil
> 5. A second loop is started because we got a new child added. Oops.
> 
> I think it's cleaner to have the deletions detected by the children watcher
> being used to stop this loop from outside, similar to what you were doing
> before. w.updates may continue to be fed from within this function only,
though,
> to avoid races and dups.

Good catch, thank you.

https://codereview.appspot.com/6354045/diff/1/state/presence/presence.go#newc...
state/presence/presence.go:433: }
On 2012/06/28 06:48:49, niemeyer wrote:
> Shouldn't it call AliveW again here? Please add a test before fixing it.

Yeah, it would seem I'm completely on crack here. Sorry stupid propose.

[fwereade, 2012-07-13 19:27:53]

Please take a look.

[fwereade, 2012-07-16 10:04:05]

Please take a look.

[dfc, 2012-07-17 00:56:22]

http://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go
File state/presence/presence.go (right):

http://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go#n...
state/presence/presence.go:444: case w.changes <- change:
Should change be set to nil after this path executes. I'm not sure what the
result of having a stale change channel here is.

http://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go#n...
state/presence/presence.go:468: stop := w.stops[key]
Is it worth putting a check here to see if stop is not nil ?

http://codereview.appspot.com/6354045/diff/10001/state/presence/presence_test.go
File state/presence/presence_test.go (right):

http://codereview.appspot.com/6354045/diff/10001/state/presence/presence_test...
state/presence/presence_test.go:335: assertNoChange := func() {
nice.

[fwereade, 2012-07-17 15:04:47]

http://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go
File state/presence/presence.go (right):

http://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go#n...
state/presence/presence.go:444: case w.changes <- change:
On 2012/07/17 00:56:22, dfc wrote:
> Should change be set to nil after this path executes. I'm not sure what the
> result of having a stale change channel here is.

I don't *think* so -- I don't see any way we can get here from the top of the
loop without having assigned to it directly.

http://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go#n...
state/presence/presence.go:468: stop := w.stops[key]
On 2012/07/17 00:56:22, dfc wrote:
> Is it worth putting a check here to see if stop is not nil ?

Again I *think* that a panic on close is the appropriate response... if it ever
happens, either we broke the implementation, or something we depend on is acting
in a way we believe it should not.

[niemeyer, 2012-07-17 15:12:22]

https://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go
File state/presence/presence.go (right):

https://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go#...
state/presence/presence.go:96: next := p.lastPing.Add(p.period)
Why is a pinger change being added to this CL?

This was added since the last review, and is not being mentioned in the
description.

https://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go#...
state/presence/presence.go:353: alive   map[string]struct{}
This should be map[string]bool, and then use w.alive[key] as a boolean in the
code below. It will look a lot nicer.

I'd also have the one below as bool as it looks nicer too.

https://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go#...
state/presence/presence.go:431: } else if _, alive := w.alive[ch.key]; alive {
} else if w.alive[key] {

https://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go#...
state/presence/presence.go:455: close(stop)
Rather than closing the channel, we can send a value down. This way the logic
will wait for it to actually stop running.

https://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go#...
state/presence/presence.go:470: close(stop)
Can also send a value down for synchronization.

https://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go#...
state/presence/presence.go:472: if _, alive := w.alive[key]; alive {
if w.alive[key] {
    ...
}
delete(w.alive, key)

https://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go#...
state/presence/presence.go:487: w.alive[key] = struct{}{}
w.alive[key] = alive

https://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go#...
state/presence/presence.go:509: aliveNow, watch, err = AliveW(w.conn, path)
The intend would be more clear by inverting the logic:

aliveNow, newWatch, err := AliveW(w.conn, path)
watch = newWatch

As we talked, it'd also be good to have a comment explaining why we do this.
Something along the lines of:

// We to watch again anyway, so do this ahead of time and
// verify that the result has indeed changed since the last
// observation, since it could have changed an even number
// of times and not be interesting otherwise.

[fwereade, 2012-07-18 08:53:45]

Please take a look.

https://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go
File state/presence/presence.go (right):

https://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go#...
state/presence/presence.go:96: next := p.lastPing.Add(p.period)
On 2012/07/17 15:12:22, niemeyer wrote:
> Why is a pinger change being added to this CL?
> 
> This was added since the last review, and is not being mentioned in the
> description.

It's not: it changed independently in the long period in which this change was
languishing because I had other stuff on my plate that I thought was more
important.

However, I did a spot of archaeology, and it looks like this change was somehow
not part of any changeset in the vast testing cleanup branch. This certainly
wasn't intentional -- I could swear I had proposed it, but the evidence denies
my memory -- but it *is* code that you haven't seen before. My best guess is
that I proposed, went away and did something else, didn't see something going
wrong with the propose, and restarted for updates or something when I came back
without checking for propose success. So, I'm sorry about that... but I think
that rectifying that issue is out of scope of this change. Thoughts?

https://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go#...
state/presence/presence.go:353: alive   map[string]struct{}
On 2012/07/17 15:12:22, niemeyer wrote:
> This should be map[string]bool, and then use w.alive[key] as a boolean in the
> code below. It will look a lot nicer.
> 
> I'd also have the one below as bool as it looks nicer too.

Went back and forth on this, happy to follow your opinion. Done.

https://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go#...
state/presence/presence.go:431: } else if _, alive := w.alive[ch.key]; alive {
On 2012/07/17 15:12:22, niemeyer wrote:
> } else if w.alive[key] {

Done.

https://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go#...
state/presence/presence.go:455: close(stop)
On 2012/07/17 15:12:22, niemeyer wrote:
> Rather than closing the channel, we can send a value down. This way the logic
> will wait for it to actually stop running.

Ah, nice, hadn't thought of that. Done.

https://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go#...
state/presence/presence.go:470: close(stop)
On 2012/07/17 15:12:22, niemeyer wrote:
> Can also send a value down for synchronization.

Done.

https://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go#...
state/presence/presence.go:472: if _, alive := w.alive[key]; alive {
On 2012/07/17 15:12:22, niemeyer wrote:
> if w.alive[key] {
>     ...
> }
> delete(w.alive, key)

Done.

https://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go#...
state/presence/presence.go:487: w.alive[key] = struct{}{}
On 2012/07/17 15:12:22, niemeyer wrote:
> w.alive[key] = alive

Done.

https://codereview.appspot.com/6354045/diff/10001/state/presence/presence.go#...
state/presence/presence.go:509: aliveNow, watch, err = AliveW(w.conn, path)
On 2012/07/17 15:12:22, niemeyer wrote:
> The intend would be more clear by inverting the logic:
> 
> aliveNow, newWatch, err := AliveW(w.conn, path)
> watch = newWatch
> 
> As we talked, it'd also be good to have a comment explaining why we do this.
> Something along the lines of:
> 
> // We to watch again anyway, so do this ahead of time and
> // verify that the result has indeed changed since the last
> // observation, since it could have changed an even number
> // of times and not be interesting otherwise.

Neat. Done.

[fwereade, 2012-07-18 09:29:47]

Please take a look.

[niemeyer, 2012-07-19 00:03:26]

LGTM!

A couple of details for your consideration:

https://codereview.appspot.com/6354045/diff/3002/state/presence/presence.go
File state/presence/presence.go (right):

https://codereview.appspot.com/6354045/diff/3002/state/presence/presence.go#n...
state/presence/presence.go:456: close(w.updates)
Why was this added? There's apparently no benefit in closing this channel.

https://codereview.appspot.com/6354045/diff/3002/state/presence/presence.go#n...
state/presence/presence.go:509: // notified, and thereby only send notifications
on real changes.
Thanks for the comment.

https://codereview.appspot.com/6354045/diff/3002/state/presence/presence.go#n...
state/presence/presence.go:520: // and we should remain silent and just wait for
the watch.
This seems a bit confusing, and is basically restating what was just said above
in a different way. I suggest dropping it.

[fwereade, 2012-07-19 07:17:02]

*** Submitted:

add presence.ChildrenWatcher

This mimics watcher.ChildrenWatcher, but expects that all children be
presence nodes; and notifies of child node Pinger life/death, rather
than mere existence of child nodes.

This is now much closer to the original proposal, but hopefully with less
apparent crack-related influences, and abandons the somewhat optimistic
(read: almost entirely composed of weapons-grade crack) attempt to make
the childLoop goroutines responsible for their own cleanup.

Original Cl at: https://codereview.appspot.com/6335057/ (CL changed for dull
prereq-related reasons).

R=niemeyer, dfc
CC=
https://codereview.appspot.com/6354045

https://codereview.appspot.com/6354045/diff/3002/state/presence/presence.go
File state/presence/presence.go (right):

https://codereview.appspot.com/6354045/diff/3002/state/presence/presence.go#n...
state/presence/presence.go:456: close(w.updates)
On 2012/07/19 00:03:26, niemeyer wrote:
> Why was this added? There's apparently no benefit in closing this channel.

The benefits is nebulous, philosophical, and arguable -- that synchronous
shutdown is a good thing, and that a maintainer who breaks it accidentally will
be informed of that by the panics, while one who's analysed the type properly
and determined that fuzzy shutdown is reasonable (which, to be fair, it actually
is; but who knows what will change) will have deleted this line deliberately.
The downside ofc is that the maintainer might break it and *not* see it while
running the tests, and end up screwing up live environments.

I'll drop it for now, let me know if you're slightly swayed by the above idea
and I'll put it back in as a trivial.

https://codereview.appspot.com/6354045/diff/3002/state/presence/presence.go#n...
state/presence/presence.go:520: // and we should remain silent and just wait for
the watch.
On 2012/07/19 00:03:26, niemeyer wrote:
> This seems a bit confusing, and is basically restating what was just said
above
> in a different way. I suggest dropping it.

Done.