Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(653)

Issue 6304068: Make watcher termination clearer, safer(?)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 10 months ago by fwereade
Modified:
11 years, 10 months ago
Reviewers:
mp+109814, rog
Visibility:
Public.

Description

Make watcher termination clearer, safer(?) The existing watcher implementations shared a flawed implementation style: specifically, that closure of the input channel was not treated as an error. This is presumably because the Stop method also stopped the watcher that provided the input channel, and this could happen at any time whatsoever (from the perspective of the loop goroutine); however, this then meant that the loop goroutine treated any of the following conditions: * watcher itself was stopped by tomb.Kill() * subwatcher was stopped by tomb.Kill() * subwatcher channel closed for some other reason ...as a sign of clean shutdown; that is to say that all subwatcher errors were being swallowed silently. This CL avoids that problem by deferring all cleanup work, including stoppage of the subwatcher, to the end of the loop goroutine; and, in the loop goroutine, treating <-w.tomb.Dying() as the only reason to terminate cleanly. Unexpected closures of the input channel are hence correctly detected as errors; and expected closures (caused by cleanup) will never be detected, because we will always have exited the goroutine loop by the time we stop the subwatcher. As a consequence, there is no need to wait on subwatcher death: if it dies while the loop is running the error will be detected soon enough via the closure of the input channel; but that's not really any reason to abort a send on the output channel. https://code.launchpad.net/~fwereade/juju-core/rework-stop-watchers/+merge/109814 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+47 lines, -49 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M state/watcher.go View 14 chunks +45 lines, -49 lines 3 comments Download

Messages

Total messages: 2
fwereade
Please take a look.
11 years, 10 months ago (2012-06-12 11:25:21 UTC) #1
rog
11 years, 10 months ago (2012-06-12 12:03:03 UTC) #2
LGTM

https://codereview.appspot.com/6304068/diff/1/state/watcher.go
File state/watcher.go (left):

https://codereview.appspot.com/6304068/diff/1/state/watcher.go#oldcode298
state/watcher.go:298: case <-w.watcher.Dying():
definite +1 on this change. as you say, a sub-watcher dying should affect a
previously received event being sent.

https://codereview.appspot.com/6304068/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6304068/diff/1/state/watcher.go#newcode199
state/watcher.go:199: if err := w.watcher.Stop(); err != nil {
as discussed on IRC, perhaps it might make sense to factor this into a function
so that the defers can be written straightforwardly without a closure.

https://codereview.appspot.com/6304068/diff/1/state/watcher.go#newcode347
state/watcher.go:347: // loop is the backend for watching the topology.
i'm not sure these comments add any useful info. shall we just delete them?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b