Code review comment for lp://staging/~rogpeppe/juju-core/simplify-watchers

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