Watcher consistency improvements

Watcher consistency improvements

* watcher fields that have no business being fields have been removed
* watcher stoppage has been consolidated
* handling of inappropriately closed watcher channels has been consolidated
* garbage watchers in cmd/jujud are now stopped explicitly
* significant code duplication removed from state/watcher.go

As a result of the final point, it has become apparent that most of the
watchers in the state package are at risk of emitting inappropriate events
that don't correspond to real changes. The fact that this is now apparent,
when it had previously been missed by every previous reviewer, is IMO a
strong indication that this change is a good move.

I haven't started to address that issue, however, because the CL is already
big enough.

https://code.launchpad.net/~fwereade/juju-core/consolidate-content-watchers/+merge/112350

(do not edit description out of merge proposal)

[fwereade, 2012-06-27 13:59:06]

Please take a look.

[fwereade, 2012-06-27 14:45:39]

Please take a look.

[niemeyer, 2012-06-28 06:11:53]

That's a great refactoring. LGTM with the following covered:

https://codereview.appspot.com/6347045/diff/7007/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6347045/diff/7007/state/watcher.go#newcode12
state/watcher.go:12: type contentWatcher struct {
All of the use cases below create this the same way. It's worth defining a
function IMO:

func newContentWatcher(st *State, path string) contentWatcher

https://codereview.appspot.com/6347045/diff/7007/state/watcher/watcher.go
File state/watcher/watcher.go (right):

https://codereview.appspot.com/6347045/diff/7007/state/watcher/watcher.go#new...
state/watcher/watcher.go:11: type Watcher interface {
// ErrStopper is implemented by all watchers.
type ErrStopper interface {

https://codereview.appspot.com/6347045/diff/7007/state/watcher/watcher.go#new...
state/watcher/watcher.go:16: // Stop is used to terminate a watcher and
propagate errors to the supplied
// Stop stops the watcher. If an error is returned by the
// watcher, t is killed with the error.

https://codereview.appspot.com/6347045/diff/7007/state/watcher/watcher.go#new...
state/watcher/watcher.go:28: // error and will therefore panic.
// MustErr returns the error with which w died.
// Calling it will panic if w is still running or was stopped cleanly.

[fwereade, 2012-06-28 08:29:21]

*** Submitted:

Watcher consistency improvements

* watcher fields that have no business being fields have been removed
* watcher stoppage has been consolidated
* handling of inappropriately closed watcher channels has been consolidated
* garbage watchers in cmd/jujud are now stopped explicitly
* significant code duplication removed from state/watcher.go

As a result of the final point, it has become apparent that most of the
watchers in the state package are at risk of emitting inappropriate events
that don't correspond to real changes. The fact that this is now apparent,
when it had previously been missed by every previous reviewer, is IMO a
strong indication that this change is a good move.

I haven't started to address that issue, however, because the CL is already
big enough.

R=niemeyer
CC=
https://codereview.appspot.com/6347045

https://codereview.appspot.com/6347045/diff/7007/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6347045/diff/7007/state/watcher.go#newcode12
state/watcher.go:12: type contentWatcher struct {
On 2012/06/28 06:11:53, niemeyer wrote:
> All of the use cases below create this the same way. It's worth defining a
> function IMO:
> 
> func newContentWatcher(st *State, path string) contentWatcher

Done.

https://codereview.appspot.com/6347045/diff/7007/state/watcher/watcher.go
File state/watcher/watcher.go (right):

https://codereview.appspot.com/6347045/diff/7007/state/watcher/watcher.go#new...
state/watcher/watcher.go:11: type Watcher interface {
On 2012/06/28 06:11:53, niemeyer wrote:
> // ErrStopper is implemented by all watchers.
> type ErrStopper interface {

Done.

https://codereview.appspot.com/6347045/diff/7007/state/watcher/watcher.go#new...
state/watcher/watcher.go:16: // Stop is used to terminate a watcher and
propagate errors to the supplied
On 2012/06/28 06:11:53, niemeyer wrote:
> // Stop stops the watcher. If an error is returned by the
> // watcher, t is killed with the error.

Done.

https://codereview.appspot.com/6347045/diff/7007/state/watcher/watcher.go#new...
state/watcher/watcher.go:28: // error and will therefore panic.
On 2012/06/28 06:11:53, niemeyer wrote:
> // MustErr returns the error with which w died.
> // Calling it will panic if w is still running or was stopped cleanly.

Done.