pkg / errors

Simple error handling primitives
https://godoc.org/github.com/pkg/errors
BSD 2-Clause "Simplified" License
8.18k stars 691 forks source link

get first stack trace info. #195

Closed wangsai-silence closed 3 years ago

wangsai-silence commented 5 years ago

For this issue https://github.com/pkg/errors/issues/173

hanzei commented 5 years ago

@wangsai-silence Could you please take a look at the comments above?

wangsai-silence commented 5 years ago

@wangsai-silence Could you please take a look at the comments above?

OK. I've done. Thank you for remind me about that.

mightyguava commented 4 years ago

Oh I was just looking for something like this. @wangsai-silence @puellanivis what is left to be done for this PR?

wangsai-silence commented 4 years ago

Oh I was just looking for something like this. @wangsai-silence @puellanivis what is left to be done for this PR?

Nobody answered. I have used it in my project.

puellanivis commented 4 years ago

🤷 I don’t have collaborator rights to the repo, so I cannot approve and merge. Also, I was going to leaving the approval/denial of the mere design up to other parties anyways.

The branch is however now out-of-date with the base branch, and likely would end up with a merge conflict or something.

Fortunately, the code works both inside of and outside of this package. So there is not much stopping anyone from replicating it. But, with proper pkg/error use sanitation, you can avoid the need to use it drastically.

flimzy commented 4 years ago

FWIW, I proposed something like this back in 2016. While it wasn't outright rejected, I closed the issue because I agreed with Dave Cheney's cautious approach.

Since then, I've written this functionality into practically every Go application I've written, and I don't mind doing that.

In fact, I expect I would continue to do so, even if this PR is merged, because, as Dave predicted, I seem to want subtly different things in each app.

And now with the advent of Go 1.13's new error handling, it's even less likely I would write an application to depend on this feature. I'd tend to prefer a version based on errors.As.

mightyguava commented 4 years ago

@flimzy ah, asking each package to implement their own version makes sense with that link. I don't understand what errors.As has to do with this though. I want to print a stacktrace for debugging.

flimzy commented 4 years ago

@flimzy ah, asking each package to implement their own version makes sense with that link. I don't understand what errors.As has to do with this though. I want to print a stacktrace for debugging.

Two things:

  1. I want a version that honors Go 1.13's wrapper interface. errors.As is the easiest way to address that.
  2. errors.As makes it trivial to implement this functionality, making the need for supporting it in a library like this less urgent, IMO. Example (untested "whiteboard" code):
stackErr := new(interface{ StackTrace() errors.StackTrace })
for errors.As(err, stackErr) {
    err = stackErr
}
if stackErr != nil {
    stack = stackErr.StackTrace()
}
mightyguava commented 4 years ago

errors.As doesn't unwrap unless necessary, so I think you have an infinite loop there.

flimzy commented 4 years ago

errors.As doesn't unwrap unless necessary, so I think you have an infinite loop there.

Possible. As I said, the code is back-of-napkin quality, untested. I don't think it affects the substance of what I was saying.

puellanivis commented 4 years ago

You are correct, errors.As does not unwrap if the given error is already assignable to the second argument. That’s easy enough to fix with an errors.Unwrap call.

But I would additionally recommend against using new() for an interface. Because an interface is already reference semantics, it is almost universally an error to make a direct datatype that is a pointer to an interface. The only time a pointer to interface is not an error, is when it is a derived type, for instance when you have a subfunction that would like to assign into an interface value, rather than just use it, where it should be derefed at the subfunction call. So, these changes get us:

func Stack(err error) StackTrace {
    var stackErr interface{ StackTrace() errors.StackTrace }
    for errors.As(err, &stackErr) {
        err = errors.Unwrap(stackErr) // err will be assignable, and infinite loop otherwise.
    }
    if stackErr != nil {
        return stackErr.StackTrace()
    }
    return nil
}
flimzy commented 4 years ago

But I would additionally recommend against using new() for an interface. ... snip ...

All good advice. Please don't get hung up on the minutiae of a throw-away coding example. I should have just left the trivial example as a mental exercise to avoid derailing the conversation. :facepalm:

puellanivis commented 4 years ago

I don’t think we derailed things. We ended up with a corrected version, that people can use. It’s the good-ol’ “say a wrong answer, and you will immediately get 50 other people to do the work of finding the correct answer for you.”