Merge lp://staging/~rogpeppe/juju-core/simplify-watchers into lp://staging/~juju/juju-core/trunk

Reviewers: mp+115070_code.launchpad.net,

Message:
Please take a look.

Description:
environs/state: simplify watcher implementation

FOR DISCUSSION ONLY

When discussing the firewall code on IRC recently, Frank proposed
some reasonable looking code that looked something like this:

func (m *machine) loop(fw *Firewaller) {
 for change := range m.watcher.Changes() {
  fw.machineUnitChanges <- machineUnitsChange{m, change}
 }
 fw.machineUnitChanges <- machineUnitsChange{m, nil}
}

Unfortunately, the way that watchers work currently, we can't write the
code that way. My counter-proposal looked a bit like this:

func (m *machine) loop(fw *Firewaller) {
 defer m.watcher.Stop()
 for {
  select {
  case <-fw.tomb.Dying():
   return
  case change, ok := <-m.watcher.Changes():
   select {
   case fw.machineUnitChanges <- machineUnitsChange{m, change}:
   case <-fw.tomb.Dying():
    return
   }
   if !ok {
    return
   }
  }
 }
}

With the firewaller and the unit agent, we will be doing a great deal
more watcher multiplexing, so we will see a *lot* of code like the
above.

I'm not happy with that - the logic is largely obscured because of the
requirement that we read from the Dying channel on every send
and receive.

I propose here that instead of each watcher having its own Stop method,
we explicitly pass in a stop channel. This means that a watcher that
layers on top of another watcher can *delegate* the stop watching
to its sub-watcher. We make any watcher error available as a
result of the Wait method (name of this method probably wrong!),
which waits for the change channel to be closed before returning
the error.

I *think* this makes the watcher logic easier to reason about, and
it also makes the first piece of code above a viable possibility again
(in fact it would probably send the error down the channel too, like
this:

func (m *machine) loop(fw *Firewaller) {
 for change := range m.watcher.Changes() {
  fw.machineUnitChanges <- machineUnitsChange{m, change, nil}
 }
 fw.machineUnitChanges <- machineUnitsChange{m, nil, m.watcher.Wait()}
}
)

Down sides of this proposal:
- it's a little more inconvenient, as you need to make a channel before
passing it to the watcher.
- You're not guaranteed to receive no more events after stopping
a watcher.
- Without a Stop method, the association between the stop channel and
the watcher is more indirect.

I don't think that any of these are significant problems, and I think
that we would benefit greatly over time, but others might beg to differ.

Thoughts? Crack?

https://code.launchpad.net/~rogpeppe/juju-core/simplify-watchers/+merge/115070

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6373048/

Affected files:
   A [revision details]
   M state/machine.go
   M state/machine_test.go
   M state/service.go
   M state/service_test.go
   M state/state.go
   M state/state_test.go
   M state/unit.go
   M state/unit_test.go
   M state/watcher.go
   M state/watcher/watcher.go
   M state/watcher/watcher_test.go
   M worker/machiner/machiner.go
   M worker/provisioner/provisioner.go

On 2012/07/16 11:06:03, rog wrote:
> Please take a look.

I think I'm still -0 on this, because of the inconvenience and lack of a
Stop method; I feel that a stop channel is not sufficiently expressive
really, because I *like* being able to know that no further changes will
arrive after a Stop, and I'd prefer to keep this somehow. In the
unit-relation-watcher branch I do something a bit like this by passing
new *Tombs to new goroutines (and using those to ensure that I never
send a depart event for a given relation unit until I know its
settings-watching goroutine has stopped, and can be counted on not to be
blocking on a send to updates (which would lead to a spurious
post-depart change event, which would in turn be interpreted as a
re-join)).

So, if we were supplying tombs (or simple wrappers thereof) I'd maybe be
+0, depending on how that made the implementation look.

https://codereview.appspot.com/6373048/

We discussed this online. I'm -1 on changing these interfaces right now
as well.

https://codereview.appspot.com/6373048/