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

Issue 6307081: 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+109877, niemeyer
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. Original discussion on https://codereview.appspot.com/6300085/ (which I appear to have proposed for the wrong target somehow). https://code.launchpad.net/~fwereade/juju-core/rework-stop-watchers/+merge/109877 (do not edit description out of merge proposal)

Patch Set 1 #

Patch Set 2 : Make watcher termination clearer, safer #

Patch Set 3 : Make watcher termination clearer, safer #

Patch Set 4 : Make watcher termination clearer, safer #

Patch Set 5 : Make watcher termination clearer, safer #

Unified diffs Side-by-side diffs Delta from patch set Stats (+34 lines, -46 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/jujud/provisioning_test.go View 1 chunk +2 lines, -2 lines 0 comments Download
M state/watcher.go View 16 chunks +30 lines, -44 lines 0 comments Download

Messages

Total messages: 3
fwereade
Please take a look.
11 years, 10 months ago (2012-06-12 15:51:11 UTC) #1
fwereade
Please take a look.
11 years, 10 months ago (2012-06-13 08:45:01 UTC) #2
niemeyer
11 years, 10 months ago (2012-06-13 14:58:19 UTC) #3
There are three different CLs with this same change, and one of them has two
LGTMs. I don't quite get how we got into that messy state, but I'm attempting to
solve it by merging https://codereview.appspot.com/6307081 and rejecting the
rest.
Sign in to reply to this message.

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