<div dir="ltr">As requested, we wanted to enable leader-elected always, and I ended up running into an unexpected failure. I tracked it down, and figured I'd solicit feedback from the group.<div><br></div><div>The specific code path is:</div><div><ol><li>leadership/tracker.go line 67 calls:<br>  t.tomb.Kill(t.loop())<br>Nothing special here, its how all our workers work.</li><li>loop() has a common case of line 137:<br>case ticketCH := <-t.waitMinionTickets:<br>  ...<br>  if err := t.resolveWaitMinion(...); err != nil {<br>    return errors.Trace(err)<br>  }</li><li>resolveWaitMinion: 290 has:<br>if leader, err := t.isLeader(); err != nil {<br>  return errors.Trace(err)<br>}<br></li><li>And isLeader()  224 has:<br>case <-t.tomb.Dying():<br>  return false, tomb.ErrDying<br><br></li></ol><div>All of which seems pretty standard. If we find out we are dying while we are trying to do something else, return ErrDying. When that bubbles up to the top, we just call tomb.Kill() with any error that we got so that if we got a genuine error, a future worker.Wait()/Stop() call can return the actual error.</div></div><div><br></div><div>However the key point is that calling:</div><div>  tomb.Kill(tomb.ErrDying)</div><div>has special code that looks at:</div><div>  if reason == ErrDying {</div><div>    ...</div><div>    return nil</div><div>  }</div><div>  t.reason = reason</div><div><br></div><div>And what is happening is that because of the errors.Trace() we aren't handing it ErrDying the singleton error instance, we are handing it an errors.Err{Cause: tomb.ErrDying}</div><div>Which then when we check "worker.Stop() error" we get a real error instead of nil.</div><div><br></div><div>So for now, I decided to trap in NewTrackerWorker and if we have an errors.Cause() of ErrDying we unwrap it before passing it to tomb.Kill().</div><div><br></div><div>If we want to be using errors.Trace more (and it certainly was nice to have at least some sort of traceback to figure out where these steps were occurring), shouldn't we be updating our general pattern?</div><div><br></div><div>Do we have to wrap every call to tomb.Kill(err) with tomb.Kill(OnlyRealErrors(err)) ?</div><div><br></div><div>Should we (can we?) teach tomb about the errors package so that it can do its own unwrapping?</div><div><br></div><div><br></div><div>uniter.go loop() has its own unwrapping check in the middle of the loop, but it isn't doing the unwrapping in NewUniter. Should I be doing the unwrapping inside loop instead? Doesn't that mean I have a lot of code paths (all of the cases in loop() conceivably could be returning an ErrDying).</div><div><br></div><div><br></div><div>Thoughts?</div><div><br>John</div><div>=:-></div><div><br></div></div>