Code review comment for lp:~themue/juju-core/go-worker-firewaller-machines

William Reade (fwereade) wrote :

I think there are several lurking gotchas in the sub-goroutines. While
I'm not *certain* that my analogous implementations in
https://codereview.appspot.com/6405044/ and
https://codereview.appspot.com/6402048/ are *bulletproof*, I'm pretty
sure they ensure events happen in the right order (ie no changes after a
depart, etc), and that this code doesn't. Ping me tomorrow if you want
to talk about it, the comments are a little scattered.

https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go#newcode77
worker/firewaller/firewaller.go:77: m.watcher.Stop()
Don't we need to wait until the *machine* has finished, not just the
watcher? ie calling m.watcher.Stop() here does *not* guarantee that we
won't see any more events from it on machineUnitsChanges: we actually
need to be sure that the machine.loop goroutine has exited.

I think we need tombs for the sub-goroutines, as I have in the
relation-units-watcher and relation-unit branches (although in his case
the tomb and Stop() should probably be on machine)

https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go#newcode129
worker/firewaller/firewaller.go:129: if !ok {
On 2012/07/16 23:23:59, fwereade wrote:
> fw.tomb.Kill(watcher.MustErr(m.watcher))?

Feels a bit over-familiar, but better than dropping errors on the floor.

https://codereview.appspot.com/6374069/

« Back to merge proposal