new dependency

[Nate Finch]

So, just three quick (ish) points:

1.) I agree we need the ability to mask errors, though how perfectly we
need to mask them is up for debate... In theory both arrar's error stack
and errgo's Cause() method expose the underlying error.  Sometimes this is
good, sometimes it's bad.  At the end of the day, it's a question of the
intent of the programmer.  There's always the option to just return a new
error that doesn't reference the lower level error, if you need or want
perfect masking of implementation errors.

2.) Arrar's annotate allows you to modify an error in-place.  I see no
problem with mutable errors, especially ones where the mutability is
contained to a specific portion of the object that is intended to be
mutated, and doesn't affect the rest of the object.  Debating whether
errors should be mutable or not is a philosophical question that doesn't
seem to matter in real code.  The only time it would matter is if you
wanted to test an error by value (like err == io.EOF), and we can just
avoid that, and check by type instead.

3.) I don't want our error handling to be non-idiomatic go.  Having to call
errgo.Diagnosis() on every error to get the real error value is
non-idiomatic.  I would much prefer that the base error object that we're
passing around actually *be* the error value.  Thus, if you pass one of our
errors into IsNotFound(err), all the function has to do is check if the
type is NotFoundError, just like normal go code would do, instead of
calling errgo's Diagnosis() or calling arrar's Check.


On Fri, Feb 7, 2014 at 1:30 PM, roger peppe <rogpeppe at gmail.com> wrote:

> On 7 February 2014 12:14, Nate Finch <nate.finch at canonical.com> wrote:
> >
> > On Fri, Feb 7, 2014 at 5:24 AM, roger peppe <rogpeppe at gmail.com> wrote:
> >>
> >>
> >> http://play.golang.org/p/Ze_YTCcfzm
> >>
> >> Note that, even ignoring the doc comment, it is easy to tell by
> >> looking at the high
> >> level function, ReadPacketFromNetwork, what classifiable errors might
> >> be returned from
> >> it. This makes it easy to write the doc comment - the contract with the
> >> caller is clear.
> >
> >
> > This seems to be where we're differing.  I'd never return
> io.UnexpectedEOF.
> > I'd return a custom error like mypackage.TruncatedData.
>
> You seem to be saying that there's no purpose in general error
> classifications. Would you have a special error type for each package
> where our errors.NotFoundError is used?
>
> Note that if you *do* want to pass your TruncatedData type back through
> several layers, you *can't* annotate it, because annotating it changes
> the type.
>
> Both errgo and arrar allow you to annotate the error (producing a new
> error value) independently of the underlying "diagnosis".
>
> The difference is that errgo considers it better behaviour to reveal
> special error types and values only when explicitly needed, and not by
> default all the time.
>
> That to me makes it more robust, because you don't by default allow
> errors to cross abstraction boundaries.
>
> > So then the calling code would look something like this:
> >
> > http://play.golang.org/p/kAWClCm3gv
> >
> > (that's using a testing function, you could instead let the caller do a
> type
> > check, but that's uglier).
> >
> > UnexpectedEOF is an implementation detail. It's the package's job to
> > translate that into something the caller can understand, and hide the
> > implementation from the caller.  What if you change the code later so it
> > calls something that doesn't return UnexpectedEOF?
>
> What if it was errors.NotFoundError ?
> Would you still consider that as something that needs to be translated
> at every level?
>
> > I guess I don't understand why you'd ever want a diagnosis that's
> different
> > from just the error you're returning.
>
> One random example (the first one I looked at, so probably not that
> unusual a case) from our source code is tools.Fetch.
>
> At least one place (tools.FindToolsForCloud) relies on it to return
> a NotFoundError if the tools are not found.  There are probably more
> places (and it's a public function, so it's quite possible some third
> party code might be using it too).
>
> It passes through the NotFoundError from an underlying call to
> simplestreams.GetMetadata. So in that case, we would almost certainly want
> to annotate the error, but we'd still want the diagnosis to be the same.
>
> But let's try to trace the code and find out where that NotFoundError is
> actually generated. I wrote down function names when I started to consider
> them, and the indentation mirrors the call tree. I stopped when I found
> the first NotFoundError return.
>
> tools.Fetch
>     simplestreams.GetMetadata
>         simplestreams.getMaybeSignedMetadata
>             GetIndexWithFormat
>                 fetchData
>                     found it!
>             IndexReference.getLatestMetadataWithFormat
>                 IndexReference.GetCloudMetadataWithFormat
>                 GetLatestMetadata
>
> So the NotFoundError is passed up 4 levels before it's actually
> acted on. In not one of those levels is it actually documented that
> a NotFoundError may be returned, and it's entirely possible that one
> of the other paths might return the same error for some other reason,
> causing the higher level logic in FindToolsForCloud to malfunction
> (or perhaps not - it's hard to know which paths are important here).
>
> This is a really good example of the kind of fragility that is a natural
> result of our current approach, and is not helped at all by arrar's
> approach (it actually makes things somewhat worse, because a former
> call to fmt.Errorf, which hides the underlying error, becomes a call to
> arrar.Annotate, which does not).
>
> Using errgo in this situation, we would be be explicit in all the places
> where we need a NotFoundError to pass through. This would make it
> very clear what paths might generate that error, even in the absence
> of documentation.
>
>
> Another good example was demonstrated by this fix:
> https://codereview.appspot.com/54950046/
>
> An error returned deep from within some worker code became conflated
> with another similar error we were expecting to find, causing the entire
> agent to be torn down rather than the worker task restarting gracefully,
> and our tests to become flaky..
>
> This was a classic abstraction-breakage failure - the error returned
> from within the worker was not returned as anything special - it
> wasn't part of the contract of that worker to return that kind of error.
> But it became confused with another error which *was* part of a contract,
> because our default behaviour is to expose rather than hide the underlying
> error values.
>
> IMO the more possible sources of error in our system, and the less that
> we are forced to explicitly declare what kind of errors might pass through
> layers of abstraction, the more this kind of problem is going to happen.
>
> > You can always add on to the stack trace when an error cross goroutine
> > boundaries.  I haven't really thought about it, so I'm not sure if you'd
> end
> > up with something really illegible or not.
>
> You end up with multiple independent stack traces, and it's very
> hard to see the actual control flow. The error wrapping path
> actually works very nicely for seeing the important information about
> the error IMO.
>
> Sorry, that turned out rather long. :-)
>
>   cheers,
>     rog.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/juju-dev/attachments/20140210/ef41a3c7/attachment.html>