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 NotifyWorker
+// interface
 type notifyWorker struct {
 
         // Internal structure
         tomb tomb.Tomb
 
         // handler is what will handle when events are triggered
-        handler WatchHandler
+        handler NotifyWatchHandler
 
-        // closedHandler is set to watcher.MustErr, but that panic()s, so we
-        // let the test suite override it.
+        // closedHandler 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.

 }
 
-// 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 interface {
-        // Wait for the NotifyWorker to finish what it is doing an exit
-        Wait() 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.

 
-        // Kill the running worker, indicating that it should stop what it is
-        // doing and exit. Killing a running worker should return error = nil
-        // from Wait.
-        Kill()
+// NotifyWatchHandler implements the business logic that is triggered
+// as part of watching a NotifyWatcher.
+type NotifyWatchHandler interface {
 
-        // Stop will call both Kill and then Wait for the worker to exit.
-        Stop() error
-}
-
-// WatchHandler implements the business logic that is triggered as part of
-// watching a NotifyWatcher.
-type WatchHandler 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.
+        // 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.
         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 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 WatchHandler, but the WatchHandler is the
-        // interesting (identifying) logic.
+        // 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 WatchHandler) NotifyWorker {
+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.

         nw := &notifyWorker{
                 handler:       handler,
                 closedHandler: 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()
 }
 
-// TearDown the handler, but ensure any error is propagated
-func handlerTearDown(handler WatchHandler, t *tomb.Tomb) {
+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.

         if err := handler.TearDown(); err != nil {
                 t.Kill(err)
         }
 }
 
 func (nw *notifyWorker) loop() error {
         var w api.NotifyWatcher
         var err error
-        defer handlerTearDown(nw.handler, &nw.tomb)
+        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 {
                 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.

                         // already have an error
                         w.Stop()
                 }
                 return err
         }
         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)
                         }
                         if err := nw.handler.Handle(); err != nil {
                                 return err
                         }
                 }
         }
-        panic("unreachable")

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

yay!

 }