worker: Introducing StringsWorker

worker/notifyworker.go

 // Copyright 2012, 2013 Canonical Ltd.
 // Licensed under the AGPLv3, see LICENCE file for details.
 
 package worker
 
 import (
         "launchpad.net/tomb"
 
         "launchpad.net/juju-core/state/api"
         "launchpad.net/juju-core/state/watcher"
 )
 
-// notifyWorker is the internal implementation of the NotifyWorker
-// interface
+// notifyWorker is the internal implementation of the Worker
+// interface, using a NotifyWatcher for handling changes.
 type notifyWorker struct {
-
-        // Internal structure
         tomb tomb.Tomb
 
         // handler is what will handle when events are triggered
         handler NotifyWatchHandler
 
-        // closedHandler is set to watcher.MustErr, but that panic()s, so
+        // mustErr is set to watcher.MustErr, but that panic()s, so
         // we let the test suite override it.
-        closedHandler func(watcher.Errer) error

[rog, 2013/08/01 11:18:47]

can we just call this mustErr, as that represents the
intent of the code?

[dimitern, 2013/08/01 12:53:54]

Done.

+        mustErr func(watcher.Errer) error
 }
-
-// NotifyWorker encapsulates the logic for a worker which is based on
-// a NotifyWatcher. We do a bit of setup, and then spin waiting for
-// the watcher to trigger or for us to be killed, and then teardown
-// cleanly.
-type NotifyWorker CommonWorker

[rog, 2013/08/01 11:18:47]

I don't think we need this type.
Let's just make NewNotifyWorker return a Worker.

[dimitern, 2013/08/01 12:53:54]

Done.

 
 // NotifyWatchHandler implements the business logic that is triggered
 // as part of watching a NotifyWatcher.
 type NotifyWatchHandler interface {
-
         // SetUp starts the handler, this should create the watcher we
         // will be waiting on for more events. SetUp can return a Watcher
-        // even if there is an error, and NotifyWorker will make sure to
-        // stop the Watcher.
+        // even if there is an error, and the notify Worker will make sure
+        // to stop the watcher.
         SetUp() (api.NotifyWatcher, error)
 
         // TearDown should cleanup any resources that are left around
         TearDown() error
 
         // Handle is called when the Watcher has indicated there are
         // changes, do whatever work is necessary to process it
         Handle() error
-
-        // String is used when reporting. It is required because
-        // NotifyWatcher is wrapping the NotifyWatchHandler, but the
-        // NotifyWatchHandler is the interesting (identifying) logic.
-        String() string

[rog, 2013/08/01 11:18:47]

I don't think we need a String method at all. It was used in the
old runTasks logic in cmd/jujud, but nothing calls
String on a worker now.

Please delete it from all relevant places - it's just clutter.

[dimitern, 2013/08/01 12:53:54]

Done.

 }
 
-// NewNotifyWorker starts a new worker running the business logic from the
-// handler. The worker loop is started in another goroutine as a side effect of
-// calling this.
-func NewNotifyWorker(handler NotifyWatchHandler) NotifyWorker {

[rog, 2013/08/01 11:18:47]

I think this should return a Worker.

[dimitern, 2013/08/01 12:53:54]

Done.

+// NewNotifyWorker starts a new worker running the business logic from
+// the handler. The worker loop is started in another goroutine as a
+// side effect of calling this.
+func NewNotifyWorker(handler NotifyWatchHandler) Worker {
         nw := &notifyWorker{
-                handler:       handler,
-                closedHandler: watcher.MustErr,
+                handler: handler,
+                mustErr: watcher.MustErr,
         }
         go func() {
                 defer nw.tomb.Done()
                 nw.tomb.Kill(nw.loop())
         }()
         return nw
 }
 
 // Kill the loop with no-error
 func (nw *notifyWorker) Kill() {
         nw.tomb.Kill(nil)
 }
 
-// Stop kils the worker and waits for it to exit
-func (nw *notifyWorker) Stop() error {

[rog, 2013/08/01 11:18:47]

I don't think we need to bother implementing this method.

[dimitern, 2013/08/01 12:53:54]

Done. As discussed, worker.Stop(x) can be used instead.

-        nw.tomb.Kill(nil)
-        return nw.tomb.Wait()
-}
-
 // Wait for the looping to finish
 func (nw *notifyWorker) Wait() error {
         return nw.tomb.Wait()
 }
 
-// String returns a nice description of this worker, taken from the underlying WatchHandler
-func (nw *notifyWorker) String() string {
-        return nw.handler.String()
+type tearDowner interface {
+        TearDown() error
 }
 
-func notifyHandlerTearDown(handler NotifyWatchHandler, t *tomb.Tomb) {

[rog, 2013/08/01 11:18:47]

how about this?

func propagateTearDown(handler interface{TearDown() error}, t *tomb.Tomb) {
    if err := handler.TearDown(); err != nil {
         t.Kill(err)
    }
}

then you won't need two differently named functions.

[dimitern, 2013/08/01 12:53:54]

Done.

-        // Tear down the handler, but ensure any error is propagated

[mue, 2013/08/01 10:35:24]

Why the comment in the method?

[dimitern, 2013/08/01 12:53:54]

Because it was describing what's being done, rather than as a ill-formatted doc
comment, as before.

+// propagateTearDown tears down the handler, but ensures any error is
+// propagated through the tomb's Kill method.
+func propagateTearDown(handler tearDowner, t *tomb.Tomb) {
         if err := handler.TearDown(); err != nil {
                 t.Kill(err)
         }
 }
 
 func (nw *notifyWorker) loop() error {
-        var w api.NotifyWatcher
-        var err error
-        defer notifyHandlerTearDown(nw.handler, &nw.tomb)

[rog, 2013/08/01 11:18:47]

I'm not convinced that it's right to call TearDown even if SetUp has returned an
error. The current logic also means that if there's a set up and then a tear
down error, only the tear down error will be seen, which seems wrong to me.

[dimitern, 2013/08/01 12:53:54]

As I understand it, you propose to call this after SetUp and report any error
from it before.

-        if w, err = nw.handler.SetUp(); err != nil {
+        w, err := nw.handler.SetUp()
+        if err != nil {
                 if w != nil {
-                        // We don't bother to propogate an error, because we

[rog, 2013/08/01 11:18:47]

s/propo/propa/

[dimitern, 2013/08/01 12:53:54]

Done.

+                        // We don't bother to propagate an error, because we
                         // already have an error
                         w.Stop()
                 }
                 return err
         }
+        defer propagateTearDown(nw.handler, &nw.tomb)
         defer watcher.Stop(w, &nw.tomb)
         for {
                 select {
                 case <-nw.tomb.Dying():
                         return tomb.ErrDying
                 case _, ok := <-w.Changes():
                         if !ok {
-                                return nw.closedHandler(w)
+                                return nw.mustErr(w)
                         }
                         if err := nw.handler.Handle(); err != nil {
                                 return err
                         }
                 }
         }
 }