Issue 6373048: environs/state: simplify watcher implementation
Descriptionenvirons/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)
Patch Set 1 #Patch Set 2 : environs/state: simplify watcher implementation #Patch Set 3 : environs/state: simplify watcher implementation #
MessagesTotal messages: 3
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
