state: simplify entity watchers

worker/firewaller/firewaller.go

 package firewaller
 
 import (
         "fmt"
         "launchpad.net/juju-core/environs"
         "launchpad.net/juju-core/environs/config"
         "launchpad.net/juju-core/log"
         "launchpad.net/juju-core/state"
         "launchpad.net/juju-core/state/watcher"
         "launchpad.net/juju-core/worker"
@@ skipping 483 matching lines @@
 type portsChange struct {
         unitd *unitData
         ports []state.Port
 }
 
 // unitData holds unit details and watches port changes.
 type unitData struct {
         tomb     tomb.Tomb
         fw       *Firewaller
         unit     *state.Unit
-        watcher  *state.UnitWatcher
         serviced *serviceData
         machined *machineData
         ports    []state.Port
 }
 
 // newUnitData returns a new data value for tracking details of the unit,
 // and starts watching the unit for port changes.
 func newUnitData(unit *state.Unit, fw *Firewaller) *unitData {
         ud := &unitData{
-                fw:      fw,
-                unit:    unit,
-                watcher: unit.Watch(),
-                ports:   make([]state.Port, 0),
+                fw:    fw,
+                unit:  unit,
+                ports: make([]state.Port, 0),
         }
         go ud.watchLoop()
         return ud
 }
 
 // watchLoop watches the unit for port changes.
 func (ud *unitData) watchLoop() {
         defer ud.tomb.Done()
-        defer ud.watcher.Stop()
+        w := ud.unit.Watch()
+        defer watcher.Stop(w, &ud.tomb)
         var ports []state.Port
         for {
                 select {
                 case <-ud.tomb.Dying():
                         return
-                case unit, ok := <-ud.watcher.Changes():
+                case _, ok := <-w.Changes():
                         if !ok {
-                                // TODO(niemeyer): Unit watcher shouldn't return a unit.
-                                err := watcher.MustErr(ud.watcher)
+                                ud.fw.tomb.Kill(watcher.MustErr(w))
+                                return
+                        }
+                        if err := ud.unit.Refresh(); err != nil {
                                 if !state.IsNotFound(err) {
                                         ud.fw.tomb.Kill(err)
                                 }
                                 return
                         }
-                        change := unit.OpenedPorts()
+                        change := ud.unit.OpenedPorts()
                         if samePorts(change, ports) {
                                 continue
                         }
                         ports = append([]state.Port(nil), change...)

[rog, 2012/11/09 10:48:58]

s/[]state.Port(nil)/ports[:0]/

no need for unnecessary allocation AFAICS.

[fwereade, 2012/11/19 09:07:20]

Done.

                         select {
                         case ud.fw.portsChange <- &portsChange{ud, change}:
                         case <-ud.tomb.Dying():
                                 return
                         }
                 }
         }
 }
 
 // samePorts returns whether old and new contain the same set of ports.
@@ skipping 20 matching lines @@
 type exposedChange struct {
         serviced *serviceData
         exposed  bool
 }
 
 // serviceData holds service details and watches exposure changes.
 type serviceData struct {
         tomb    tomb.Tomb
         fw      *Firewaller
         service *state.Service
-        watcher *state.ServiceWatcher
         exposed bool
         unitds  map[string]*unitData
 }
 
 // newServiceData returns a new data value for tracking details of the
 // service, and starts watching the service for exposure changes.
 func newServiceData(service *state.Service, fw *Firewaller) *serviceData {
         sd := &serviceData{
                 fw:      fw,
                 service: service,
-                watcher: service.Watch(),
                 unitds:  make(map[string]*unitData),
         }
         sd.exposed = service.IsExposed()
         go sd.watchLoop(sd.exposed)

[rog, 2012/11/09 10:48:58]

perhaps we could consider using the pattern that's becoming prevalent elsewhere
- make watchLoop return an error, and kill the tomb with that error. this means
the tomb.Kill lines below can just return the error.

[fwereade, 2012/11/19 09:07:20]

I'm not sure the pattern quite fits here -- we don't always kill on the way out
of that method.

         return sd
 }
 
 // watchLoop watches the service's exposed flag for changes.
 func (sd *serviceData) watchLoop(exposed bool) {
         defer sd.tomb.Done()
-        defer sd.watcher.Stop()
+        w := sd.service.Watch()
+        defer watcher.Stop(w, &sd.tomb)
         for {
                 select {
                 case <-sd.tomb.Dying():
                         return
-                case service, ok := <-sd.watcher.Changes():
+                case _, ok := <-w.Changes():
                         if !ok {
-                                // TODO(niemeyer): Service watcher shouldn't return a service.
-                                err := watcher.MustErr(sd.watcher)
+                                sd.fw.tomb.Kill(watcher.MustErr(w))
+                                return
+                        }
+                        if err := sd.service.Refresh(); err != nil {
                                 if !state.IsNotFound(err) {
                                         sd.fw.tomb.Kill(err)
                                 }
                                 return
                         }
-                        change := service.IsExposed()
+                        change := sd.service.IsExposed()
                         if change == exposed {
                                 continue
                         }
                         exposed = change
                         select {
                         case sd.fw.exposedChange <- &exposedChange{sd, change}:
                         case <-sd.tomb.Dying():
                                 return
                         }
                 }
@@ skipping 12 matching lines @@
         for _, a := range A {
                 for _, b := range B {
                         if a == b {
                                 continue next
                         }
                 }
                 missing = append(missing, a)
         }
         return
 }