Skip to content
This repository has been archived by the owner on Dec 1, 2021. It is now read-only.

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

Open
brianbwong opened this issue May 31, 2018 · 3 comments
Open

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

brianbwong opened this issue May 31, 2018 · 3 comments

Comments

@brianbwong
Copy link

brianbwong commented May 31, 2018

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!

@brianbwong brianbwong changed the title Wrap() duplicates stack if err is a causer Wrap() duplicates call stack if err is already a causer May 31, 2018
@brianbwong brianbwong changed the title Wrap() duplicates call stack if err is already a causer WriteStack() duplicates call stack if err is already a causer May 31, 2018
@davecheney
Copy link
Member

davecheney commented May 31, 2018 via email

@brianbwong
Copy link
Author

brianbwong commented May 31, 2018

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.

sagikazarmark added a commit to emperror/emperror that referenced this issue Aug 30, 2018
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.
@Sherlock-Holo
Copy link
Contributor

@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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants