On 24 August 2015 at 19:22, Nate Finch <nate.fi...@canonical.com> wrote:
> That's fair. I wasn't sure if recovering the panic in errors would be a
> good idea, since it could hide programmer error (like what I'd done). But
> I'm fine with that solution.
To be fair, I'm not entirely sure that panic recovery is a good idea
either, but fmt sets the
precedent to follow here.
> On Mon, Aug 24, 2015 at 3:22 AM David Cheney <david.che...@canonical.com>
> wrote:
>>
>> Yup, I agree with Rog. This is fixing the problem in the wrong place.
>>
>> On Mon, Aug 24, 2015 at 5:14 PM, roger peppe <roger.pe...@canonical.com>
>> wrote:
>> > I'm afraid I'm not convinced. Declaring the Error method on the
>> > pointer receiver is idiomatic (just grep for ' Error\(' in the Go
>> > source)
>> > and is a useful indicator that the error value is always intended
>> > to be a pointer.
>> >
>> > There's a much simpler fix for this: let the errors package
>> > recover from this itself. We can just make Err.Error call fmt.Sprint
>> > to get the error message (a one line change)
>> >
>> > Then a wrapped nil error will print <nil> just like normal nil
>> > errors.
>> >
>> >
>> > On 20 August 2015 at 03:45, Nate Finch <nate.fi...@canonical.com> wrote:
>> >> tl;dr: Don't. Use a value receiver. 99% of the time you can just
>> >> delete
>> >> the * on the receiver and it'll still work. If you really must use a
>> >> pointer, please handle the case where you're called with a nil
>> >> receiver.
>> >>
>> >> I spent most of today trying to understand why my new hook command was
>> >> producing this output:
>> >>
>> >> error: %!v(PANIC=runtime error: invalid memory address or nil pointer
>> >> dereference)
>> >>
>> >> It took me a while to figure out that this is what fmt.Printf("error:
>> >> %v\n",
>> >> err) outputs when err's Error() method panics. If you're using %s or
>> >> %v to
>> >> print a value (or use Println which implicitly uses %v), then fmt will
>> >> look
>> >> for a String() method or Error() method on the value to call, and use
>> >> the
>> >> output of that for the value's string output. If that method panics,
>> >> fmt
>> >> prints the panic in the way you see above (everything after the
>> >> PANIC=).
>> >>
>> >> Of course, the problem here is that there's no type being written, and
>> >> since
>> >> it was an error interface, it could almost anything. Using %#v skips
>> >> calling the Error/String methods and prints out the values in a go
>> >> format,
>> >> which told me this was a juju/errors.Err value, wrapping an API params
>> >> Error
>> >> value which was a nil pointer. When we call Error() on an errors.Err,
>> >> we
>> >> call Error() on the cause explicitly, which was panicking.
>> >>
>> >> Here's a minimal reproduction http://play.golang.org/p/ncNVrza-hn
>> >> (you'll
>> >> have to copy it to a local file and go run it, since the playground
>> >> won't
>> >> run code external to the stdlib).
>> >>
>> >> So what's sort of interesting is that printing the error before it gets
>> >> Traced works fine, but after the trace it is not fine. The
>> >> errors.Err's
>> >> Error() function looks like it's explicitly calling the Error() method
>> >> on
>> >> the wrapped Cause error, which is probably the problem. fmt.Printf
>> >> must use
>> >> some reflection magic to avoid doing that.
>> >>
>> >> Now, the root cause of this particular bug is actually my own mistake.
>> >> Line
>> >> 21 should check if orig is nil and then assign nil explicitly to err if
>> >> it
>> >> is. Then errors.Trace would be able to tell that the error is nil and
>> >> would
>> >> just return nil itself, instead of thinking it's a valid error and
>> >> wrapping
>> >> it.
>> >>
>> >> However, you can sidestep this entirely by doing one of two things:
>> >> either
>> >> just make the Error() (or String()) method use a value receiver.. in
>> >> which
>> >> case this code would produce this output:
>> >>
>> >> %!v(PANIC=value method main.MyError.Error called using nil *MyError
>> >> pointer)
>> >>
>> >> (you can try it with the repro code I linked to)
>> >>
>> >> This printout is a lot more helpful and useful and obvious than the
>> >> other
>> >> "nil pointer" printout.
>> >>
>> >> OR
>> >>
>> >> Just handle a nil receiver:
>> >>
>> >> func (e *MyError) Error() string {
>> >> if e == nil {
>> >> return "<nil MyError>"
>> >> }
>> >> return e.Message
>> >> }
>> >>
>> >> (note that it is dereferencing the pointer to e to access the Message
>> >> field
>> >> which causes the panic. Calling a method on a nil pointer is totally
>> >> fine
>> >> and will not panic if the code inside does not try to derefence the
>> >> pointer
>> >> to get to a field).
>> >>
>> >> Grepping through our code, I see a lot of pointer receivers on Error
>> >> and
>> >> String methods (45 and 77 respectively). I think we should at least
>> >> change
>> >> all of these to be value methods (unless that's not possible. That's
>> >> a
>> >> trivial change, and gives a much more useful printout when the pointer
>> >> is
>> >> nil.
>> >>
>> >> -Nate
>> >>
>> >> --
>> >> Juju-dev mailing list
>> >> Juju-dev@lists.ubuntu.com
>> >> Modify settings or unsubscribe at:
>> >> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>> >>
>> >
>> > --
>> > Juju-dev mailing list
>> > Juju-dev@lists.ubuntu.com
>> > Modify settings or unsubscribe at:
>> > https://lists.ubuntu.com/mailman/listinfo/juju-dev
--
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/juju-dev
