state/api/watcher: Client-side watchers improved

state/api/watcher: Client-side watchers improved

This branch fixes an issue with StringsWatcher at
client-side: it was possible to drop events if you
do not keep reading constantly. Now they get queued
up and arrive in the order delivered when reading.
Added a test as well.

Also, as a drive-by fix: there was a possibility for
deadlocking in both StringsWatcher and NotifyWatcher
when trying to close it while there is an event waiting
to be read. Added tests for these cases as well and
verified that without the fix deadlocks are indeed
triggered.

https://code.launchpad.net/~dimitern/juju-core/071-stringswatcher-client-improvements/+merge/175485

(do not edit description out of merge proposal)

[dimitern, 2013-07-18 10:01:06]

Please take a look.

[fwereade, 2013-07-18 10:27:35]

A few thoughts, let me know yours.

https://codereview.appspot.com/11506043/diff/1/state/api/watcher/watcher.go
File state/api/watcher/watcher.go (left):

https://codereview.appspot.com/11506043/diff/1/state/api/watcher/watcher.go#o...
state/api/watcher/watcher.go:225: panic("unreachable")
I *think* that we can tweak the commonWatcher loop so that we deliver the
initial event first, and only call Next again once that's been delivered. Sane?

https://codereview.appspot.com/11506043/diff/1/state/api/watcher/watcher.go
File state/api/watcher/watcher.go (right):

https://codereview.appspot.com/11506043/diff/1/state/api/watcher/watcher.go#n...
state/api/watcher/watcher.go:210: in := w.in
Surely this should be `var in <-chan []string`? Otherwise we're not
flip-flopping for the first event.

https://codereview.appspot.com/11506043/diff/1/state/api/watcher/watcher_test.go
File state/api/watcher/watcher_test.go (right):

https://codereview.appspot.com/11506043/diff/1/state/api/watcher/watcher_test...
state/api/watcher/watcher_test.go:144: // units.
...inducing an update server-side in between the two changes to ensure they're
reported as separate events over the API?

https://codereview.appspot.com/11506043/diff/1/state/api/watcher/watcher_test...
state/api/watcher/watcher_test.go:156: wc.AssertMoreChanges("logging/0")
Would AssertChange read better here perhaps?

https://codereview.appspot.com/11506043/diff/1/state/testing/watcher.go
File state/testing/watcher.go (left):

https://codereview.appspot.com/11506043/diff/1/state/testing/watcher.go#oldco...
state/testing/watcher.go:155: }
Yeah, I think it's a little bit cleaner if you:

rename AssertOneChange -> AssertChange
remove the call to AssertNoChange
add a new AssertOneChange that just calls AssertChange then AssertNoChange

sane?

[dimitern, 2013-07-18 10:52:57]

Please take a look.

https://codereview.appspot.com/11506043/diff/1/state/api/watcher/watcher.go
File state/api/watcher/watcher.go (left):

https://codereview.appspot.com/11506043/diff/1/state/api/watcher/watcher.go#o...
state/api/watcher/watcher.go:225: panic("unreachable")
On 2013/07/18 10:27:36, fwereade wrote:
> I *think* that we can tweak the commonWatcher loop so that we deliver the
> initial event first, and only call Next again once that's been delivered.
Sane?

I'm not sure this is necessary with the change having the local in chan
initially nil in StringsWatcher. If we need to do that, we can implement it
later. And the way StringsWatcher works once started we always have the initial
event and can queue it for sending, so changing the commonWatcher logic won't
affect that.

https://codereview.appspot.com/11506043/diff/1/state/api/watcher/watcher.go
File state/api/watcher/watcher.go (right):

https://codereview.appspot.com/11506043/diff/1/state/api/watcher/watcher.go#n...
state/api/watcher/watcher.go:210: in := w.in
On 2013/07/18 10:27:36, fwereade wrote:
> Surely this should be `var in <-chan []string`? Otherwise we're not
> flip-flopping for the first event.

It actually is "var in chan interface{}", but good point. Done.

https://codereview.appspot.com/11506043/diff/1/state/api/watcher/watcher_test.go
File state/api/watcher/watcher_test.go (right):

https://codereview.appspot.com/11506043/diff/1/state/api/watcher/watcher_test...
state/api/watcher/watcher_test.go:144: // units.
On 2013/07/18 10:27:36, fwereade wrote:
> ...inducing an update server-side in between the two changes to ensure they're
> reported as separate events over the API?

Done.

https://codereview.appspot.com/11506043/diff/1/state/api/watcher/watcher_test...
state/api/watcher/watcher_test.go:156: wc.AssertMoreChanges("logging/0")
On 2013/07/18 10:27:36, fwereade wrote:
> Would AssertChange read better here perhaps?

Done.

https://codereview.appspot.com/11506043/diff/1/state/testing/watcher.go
File state/testing/watcher.go (left):

https://codereview.appspot.com/11506043/diff/1/state/testing/watcher.go#oldco...
state/testing/watcher.go:155: }
On 2013/07/18 10:27:36, fwereade wrote:
> Yeah, I think it's a little bit cleaner if you:
> 
> rename AssertOneChange -> AssertChange
> remove the call to AssertNoChange
> add a new AssertOneChange that just calls AssertChange then AssertNoChange
> 
> sane?

Done.

[fwereade, 2013-07-18 11:02:10]

LGTM, just suggestions.

https://codereview.appspot.com/11506043/diff/1/state/api/watcher/watcher.go
File state/api/watcher/watcher.go (left):

https://codereview.appspot.com/11506043/diff/1/state/api/watcher/watcher.go#o...
state/api/watcher/watcher.go:225: panic("unreachable")
On 2013/07/18 10:52:57, dimitern wrote:
> On 2013/07/18 10:27:36, fwereade wrote:
> > I *think* that we can tweak the commonWatcher loop so that we deliver the
> > initial event first, and only call Next again once that's been delivered.
> Sane?
> 
> I'm not sure this is necessary with the change having the local in chan
> initially nil in StringsWatcher. If we need to do that, we can implement it
> later. And the way StringsWatcher works once started we always have the
initial
> event and can queue it for sending, so changing the commonWatcher logic won't
> affect that.

I agree it's not *necessary*, but it does ensure we don't hit the API until
we've got a good reason to, and anything that makes the server have to work
slightly less hard feels like a win to me. Up to you -- but if we can slightly
lighten the load without changing externally observable behaviour, why not? :)

https://codereview.appspot.com/11506043/diff/7001/state/api/watcher/watcher_t...
File state/api/watcher/watcher_test.go (right):

https://codereview.appspot.com/11506043/diff/7001/state/api/watcher/watcher_t...
state/api/watcher/watcher_test.go:144: // units, inducing an update server-side
in between the two
s/in between the two/after each/

? (sorry)

[dimitern, 2013-07-18 14:44:00]

Please take a look.

https://codereview.appspot.com/11506043/diff/1/state/api/watcher/watcher.go
File state/api/watcher/watcher.go (left):

https://codereview.appspot.com/11506043/diff/1/state/api/watcher/watcher.go#o...
state/api/watcher/watcher.go:225: panic("unreachable")
On 2013/07/18 11:02:10, fwereade wrote:
> On 2013/07/18 10:52:57, dimitern wrote:
> > On 2013/07/18 10:27:36, fwereade wrote:
> > > I *think* that we can tweak the commonWatcher loop so that we deliver the
> > > initial event first, and only call Next again once that's been delivered.
> > Sane?
> > 
> > I'm not sure this is necessary with the change having the local in chan
> > initially nil in StringsWatcher. If we need to do that, we can implement it
> > later. And the way StringsWatcher works once started we always have the
> initial
> > event and can queue it for sending, so changing the commonWatcher logic
won't
> > affect that.
> 
> I agree it's not *necessary*, but it does ensure we don't hit the API until
> we've got a good reason to, and anything that makes the server have to work
> slightly less hard feels like a win to me. Up to you -- but if we can slightly
> lighten the load without changing externally observable behaviour, why not? :)

Done as agreed online and added tests to detect possible deadlocks with blocking
send + stop.

https://codereview.appspot.com/11506043/diff/7001/state/api/watcher/watcher_t...
File state/api/watcher/watcher_test.go (right):

https://codereview.appspot.com/11506043/diff/7001/state/api/watcher/watcher_t...
state/api/watcher/watcher_test.go:144: // units, inducing an update server-side
in between the two
On 2013/07/18 11:02:10, fwereade wrote:
> s/in between the two/after each/
> 
> ? (sorry)

Done.

[rog, 2013-07-18 15:19:24]

LGTM assuming you got the watcher deadlock test to fail before applying the new
changes.

https://codereview.appspot.com/11506043/diff/7003/state/api/watcher/watcher.go
File state/api/watcher/watcher.go (right):

https://codereview.appspot.com/11506043/diff/7003/state/api/watcher/watcher.g...
state/api/watcher/watcher.go:157: break
maybe return nil for symmetry here and panic below the loop.
alternatively add a loop label and use break label.

https://codereview.appspot.com/11506043/diff/7003/state/api/watcher/watcher.g...
state/api/watcher/watcher.go:203: // Send changes when we have them.
// Send the initial event or subsequent change.

?

https://codereview.appspot.com/11506043/diff/7003/state/api/watcher/watcher.g...
state/api/watcher/watcher.go:208: // Then read the next event, so we ensure the
order of delivery.
// Read the next change.
?

https://codereview.appspot.com/11506043/diff/7003/state/testing/watcher.go
File state/testing/watcher.go (right):

https://codereview.appspot.com/11506043/diff/7003/state/testing/watcher.go#ne...
state/testing/watcher.go:27: func AssertStoppedWhenSending(c *C, stopper
Stopper) {
AssertCanStopWhenSending ?

https://codereview.appspot.com/11506043/diff/7003/state/testing/watcher.go#ne...
state/testing/watcher.go:34: AssertStop(c, stopper)
you shouldn't use assert in goroutines outside the main gocheck test.

   c.Check(stopper.Stop(), IsNil)

instead.

https://codereview.appspot.com/11506043/diff/7003/state/testing/watcher.go#ne...
state/testing/watcher.go:35: stopped = true
this is racy.

use a chan bool instead.

https://codereview.appspot.com/11506043/diff/7003/state/testing/watcher.go#ne...
state/testing/watcher.go:183: func (c StringsWatcherC) AssertOneChange(expect
...string) {
"OneChange" doesn't seem quite right when there are an arbitrary number.
AssertChangesAndNoMore ?

tbh i think i'd lose this function - it's only an extra line for any test code
to implement; i don't think it's worth its weight.

[rog, 2013-07-18 15:20:10]

On 2013/07/18 15:19:24, rog wrote:
> LGTM assuming you got the watcher deadlock test to fail before applying the
new
> changes.

and assuming you're ok with the more substantive suggestions
below.

> 
> https://codereview.appspot.com/11506043/diff/7003/state/api/watcher/watcher.go
> File state/api/watcher/watcher.go (right):
> 
>
https://codereview.appspot.com/11506043/diff/7003/state/api/watcher/watcher.g...
> state/api/watcher/watcher.go:157: break
> maybe return nil for symmetry here and panic below the loop.
> alternatively add a loop label and use break label.
> 
>
https://codereview.appspot.com/11506043/diff/7003/state/api/watcher/watcher.g...
> state/api/watcher/watcher.go:203: // Send changes when we have them.
> // Send the initial event or subsequent change.
> 
> ?
> 
>
https://codereview.appspot.com/11506043/diff/7003/state/api/watcher/watcher.g...
> state/api/watcher/watcher.go:208: // Then read the next event, so we ensure
the
> order of delivery.
> // Read the next change.
> ?
> 
> https://codereview.appspot.com/11506043/diff/7003/state/testing/watcher.go
> File state/testing/watcher.go (right):
> 
>
https://codereview.appspot.com/11506043/diff/7003/state/testing/watcher.go#ne...
> state/testing/watcher.go:27: func AssertStoppedWhenSending(c *C, stopper
> Stopper) {
> AssertCanStopWhenSending ?
> 
>
https://codereview.appspot.com/11506043/diff/7003/state/testing/watcher.go#ne...
> state/testing/watcher.go:34: AssertStop(c, stopper)
> you shouldn't use assert in goroutines outside the main gocheck test.
> 
>    c.Check(stopper.Stop(), IsNil)
> 
> instead.
> 
>
https://codereview.appspot.com/11506043/diff/7003/state/testing/watcher.go#ne...
> state/testing/watcher.go:35: stopped = true
> this is racy.
> 
> use a chan bool instead.
> 
>
https://codereview.appspot.com/11506043/diff/7003/state/testing/watcher.go#ne...
> state/testing/watcher.go:183: func (c StringsWatcherC) AssertOneChange(expect
> ...string) {
> "OneChange" doesn't seem quite right when there are an arbitrary number.
> AssertChangesAndNoMore ?
> 
> tbh i think i'd lose this function - it's only an extra line for any test code
> to implement; i don't think it's worth its weight.

[dimitern, 2013-07-18 15:39:36]

Please take a look.

https://codereview.appspot.com/11506043/diff/7003/state/api/watcher/watcher.go
File state/api/watcher/watcher.go (right):

https://codereview.appspot.com/11506043/diff/7003/state/api/watcher/watcher.g...
state/api/watcher/watcher.go:157: break
On 2013/07/18 15:19:25, rog wrote:
> maybe return nil for symmetry here and panic below the loop.
> alternatively add a loop label and use break label.

Will return nil instead.

https://codereview.appspot.com/11506043/diff/7003/state/api/watcher/watcher.g...
state/api/watcher/watcher.go:203: // Send changes when we have them.
On 2013/07/18 15:19:25, rog wrote:
> // Send the initial event or subsequent change.
> 
> ?

Done.

https://codereview.appspot.com/11506043/diff/7003/state/api/watcher/watcher.g...
state/api/watcher/watcher.go:208: // Then read the next event, so we ensure the
order of delivery.
On 2013/07/18 15:19:25, rog wrote:
> // Read the next change.
> ?

Done.

https://codereview.appspot.com/11506043/diff/7003/state/testing/watcher.go
File state/testing/watcher.go (right):

https://codereview.appspot.com/11506043/diff/7003/state/testing/watcher.go#ne...
state/testing/watcher.go:27: func AssertStoppedWhenSending(c *C, stopper
Stopper) {
On 2013/07/18 15:19:25, rog wrote:
> AssertCanStopWhenSending ?

Done.

https://codereview.appspot.com/11506043/diff/7003/state/testing/watcher.go#ne...
state/testing/watcher.go:34: AssertStop(c, stopper)
On 2013/07/18 15:19:25, rog wrote:
> you shouldn't use assert in goroutines outside the main gocheck test.
> 
>    c.Check(stopper.Stop(), IsNil)
> 
> instead.

Done.

https://codereview.appspot.com/11506043/diff/7003/state/testing/watcher.go#ne...
state/testing/watcher.go:35: stopped = true
On 2013/07/18 15:19:25, rog wrote:
> this is racy.
> 
> use a chan bool instead.

Done.

https://codereview.appspot.com/11506043/diff/7003/state/testing/watcher.go#ne...
state/testing/watcher.go:183: func (c StringsWatcherC) AssertOneChange(expect
...string) {
On 2013/07/18 15:19:25, rog wrote:
> "OneChange" doesn't seem quite right when there are an arbitrary number.
> AssertChangesAndNoMore ?
> 
> tbh i think i'd lose this function - it's only an extra line for any test code
> to implement; i don't think it's worth its weight.

I was thinking about removing it. Ok, will do.