WriteStack() duplicates call stack if err is already a causer

[brianbwong]

package main

import (
    "fmt"
    "github.com/pkg/errors"
)

func GiveError() error {
    return errors.New("this is an error")
}

func Outer() error {
    err := GiveError()
    return errors.WithStack(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:18
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
main.Outer
	/Users/brianwong/go/src/github.com/brianbwong/test.go:14
main.main
	/Users/brianwong/go/src/github.com/brianbwong/test.go:18
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).

If this isn't intended, here's a rough proposal for a fix:

// WithStack annotates err with a stack trace at the point WithStack was called.
// If err is nil, WithStack returns nil.
func WithStack(err error) error {
	if err == nil {
		return nil
	}

    _, ok := err.(causer)
    if ok {
        return err
    }

	return &withStack{
		err,
		callers(),
	}
}

Though currently it looks like the type fundamental isn't actually a causer.

Happy to submit a PR if desired!

[davecheney]

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.

[brianbwong]

Thanks for the reply. nmiyake's post in this issue best sums up my point.

#76 has merged, and I think it solves the issue in most cases -- basically, call Wrap or New when error is first generated, and WithMessage to annotate further.

I think it would be nice/useful if there was an implementation of WithMessage that adds a message and also adds a stack to the error if there is no error in the causal chain that already has a stack. Otherwise, to avoid stack duplication, the person writing the code has to know that the original error they're wrapping doesn't already have a stack attached to it. This is fine for things like the standard library, but if there are multiple libraries/packages that use this package, then if you don't want to duplicate stack entries (but still want to add context) you either need to do this check manually at every point or inspect the call stack all the way down manually. This issue also becomes more prevalent as more libraries adopt the package.

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 stack.

[Sherlock-Holo]

@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