-
Notifications
You must be signed in to change notification settings - Fork 697
WriteStack() duplicates call stack if err is already a causer #158
Comments
Thank you for raising this issue. WithStack and WithMessage were added to allow the caller to add just a message or a stack trace if they want. In retrospect Wrap was a mistake.
Please see the many other open issues for discussions on adding stacks if not already present or combining stack traces.
… On 1 Jun 2018, at 05:47, Brian Wong ***@***.***> wrote:
`
package main
import (
"fmt"
"github.com/pkg/errors"
)
func GiveError() error {
return errors.New("this is an error")
}
func Outer() error {
err := GiveError()
// fmt.Printf("%+v\n\n\n", errors.Cause(err))
return errors.Wrap(err, "wrapper")
// return errors.Wrap(errors.Cause(err), "")
}
func main() {
err := Outer()
fmt.Printf("%+v\n", err)
}
`
Output:
this is an error main.GiveError /Users/brianwong/go/src/github.com/brianbwong/test.go:9 main.Outer /Users/brianwong/go/src/github.com/brianbwong/test.go:13 main.main /Users/brianwong/go/src/github.com/brianbwong/test.go:20 runtime.main /usr/local/opt/go/libexec/src/runtime/proc.go:198 runtime.goexit /usr/local/opt/go/libexec/src/runtime/asm_amd64.s:2361 wrapper main.Outer /Users/brianwong/go/src/github.com/brianbwong/test.go:15 main.main /Users/brianwong/go/src/github.com/brianbwong/test.go:20 runtime.main /usr/local/opt/go/libexec/src/runtime/proc.go:198 runtime.goexit /usr/local/opt/go/libexec/src/runtime/asm_amd64.s:2361
In this toy example, it's obvious that the output of GiveError() already contains a stack trace, so there's no point in Wrapping it. However, if GiveError() is a function in an external package, it won't be clear whether its output contains a stack trace or not (and hence whether it needs wrapping).
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Thanks for the reply. nmiyake's post in this issue best sums up my point.
Is your opinion still that it is not worthwhile to implement this functionality? If so, what is the best practice for handling an error from another package? Is there a clean way to check whether this received error is a stdlib error that needs a call stack or one from this package that already has a call stack attached to it? It seems that it's much more natural to implement this check within the pkg/errors code, where we can access the underlying field |
Currently github.com/pkg/errors.Wrap/Wrapf functions overwrite already attached stack trace. From a UX/DX point of view one would like to add stack trace to an error if none is attached yet. There are several open issues discussing the problem in the original repo: pkg/errors#75 pkg/errors#144 pkg/errors#158 At the moment there is no solution provided, but it looks like the author is willing to make some changes. Until then this commit adds custom Wrap/Wrapf functions which implement this desired behaviour. Other than that, they behave the same way. There is also a pending PR in the original repo: pkg/errors#122 It worths mentioning that there are alternative solutions, like merging stack traces: srvc/fail#13 After examining the solution it seems that it's probably too complicated and unnecessary for most cases. It's also unlikely that the original errors package will implement this behaviour, so for now we stick to the most expected behaviour.
@brianbwong I very agree with your opinion because I think the stack message is used to help us to debug codes and find out which line causes this error quickly, but if it displays a lot of duplicates stack message, it will waste time to find out the real stack message and waste disk space to store log |
Output:
In this toy example, it's obvious that the output of GiveError() already contains a stack trace, so there's no point in Wrapping it. However, if GiveError() is a function in an external package, it won't be clear whether its output contains a stack trace or not (and hence whether it needs wrapping).
If this isn't intended, here's a rough proposal for a fix:
Though currently it looks like the type
fundamental
isn't actually acauser
.Happy to submit a PR if desired!
The text was updated successfully, but these errors were encountered: