Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Correct errors.WithStack behaviour #984

Merged
merged 8 commits into from
Dec 20, 2022
Merged

fix: Correct errors.WithStack behaviour #984

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
357a46e
Correct typo
AndrewSisley Dec 19, 2022
cf3f478
Remove --race ommision from TestWithStack
AndrewSisley Dec 19, 2022
4e30081
Correctly handle varying stack depth generated within this library
AndrewSisley Dec 19, 2022
5ae9133
Include ommited test assertions
AndrewSisley Dec 19, 2022
ada8cfc
Block inlining of other stack-sensitive functions
AndrewSisley Dec 19, 2022
dae4290
PR FIXUP - Rename depthToSkip
AndrewSisley Dec 20, 2022
f35f269
PR FIXUP - Document withStackTrace
AndrewSisley Dec 20, 2022
bf3d13d
PR FIXUP - Remove newError
AndrewSisley Dec 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 25 additions & 8 deletions errors/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,20 @@ func NewKV(key string, value any) KV {
// pairs provided.
//
// A stacktrace will be yielded if formatting with a `+`, e.g `fmt.Sprintf("%+v", err)`.
// This function will not be inlined by the compiler as it will spoil any stacktrace
// generated.
//go:noinline
func New(message string, keyvals ...KV) error {
return newError(message, keyvals...)
return newError(message, 1, keyvals...)
}

// Wrap creates a new error of the given message that contains
// the given inner error, suffixing any key-value pairs provided.
// This function will not be inlined by the compiler as it will spoil any stacktrace
// generated.
//go:noinline
func Wrap(message string, inner error, keyvals ...KV) error {
err := newError(message, keyvals...)
err := newError(message, 1, keyvals...)
err.inner = inner
return err
}
Expand All @@ -54,19 +60,30 @@ func Is(err, target error) bool {
return errors.Is(err, target)
}

// This function will not be inlined by the compiler as it will spoil any stacktrace
// generated.
//go:noinline
func WithStack(err error, keyvals ...KV) error {
return withStackTrace(err.Error(), keyvals...)
return withStackTrace(err.Error(), 1, keyvals...)
}

func newError(message string, keyvals ...KV) *defraError {
return withStackTrace(message, keyvals...)
// This function will not be inlined by the compiler as it will spoil any stacktrace
// generated.
//go:noinline
func newError(message string, additionalStackDepthToSkip int, keyvals ...KV) *defraError {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo: If newError will be kept, would be nice to have a line of documentation for the additionalStackDepthToSkip parameter.

return withStackTrace(message, additionalStackDepthToSkip+1, keyvals...)
}

func withStackTrace(message string, keyvals ...KV) *defraError {
// This function will not be inlined by the compiler as it will spoil any stacktrace
// generated.
//go:noinline
func withStackTrace(message string, additionalStackDepthToSkip int, keyvals ...KV) *defraError {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: I see why you've introduce this new parameter but I think we should avoid it. If we add error creation functions in the future, it requires knowledge of what this means which I think shouldn't be necessary.

What I would do instead is remove the newError function and use withStackTrace in its place. With a note on the function doc string that this has to be called within the body of the error creating function, we can leave the depth to a fixed number and avoid future dev confusion.

Copy link
Contributor Author

@AndrewSisley AndrewSisley Dec 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That wouldn't protect against the same problem being introduced by new functions, and hides the bug in the same way that it was hidden before. The new parameter forces visibility of the relationship between parent-child functions and the resultant number. It also gives us a natural route to expose this parameter if we ever find the need to.

If we add error creation functions in the future, it requires knowledge of what this means

This knowledge is required either way if they want a correct stacktrace (unless it accidentally has the same depth, which is exactly what caused this bug in the first place)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cognitive requirements are much lower if we don't need to calculate the depth where we are at when writing the function. Asking to have the withStackTrace function within the error creating function is trivial. Plus our tests should catch a wrong depths with the change you've made no?

newError can probably go regardless of keeping the parameter or not.

Copy link
Contributor Author

@AndrewSisley AndrewSisley Dec 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus our tests should catch a wrong depths with the change you've made no?

Not for new public functions - they would require new tests that explicitly test the stacktrace. Assuming that those will always be written is wishful thinking IMO.

Cognitive requirements are much lower...

The cognitive requirements exist either way, they were just hidden in the old version. Which is pretty much the only reason the bug existed (plus poor testing ofc - somewhat validating my 'wishful thinking' point above).

newError could be dropped sure, and withStackTrace should probably be renamed, but that is out of scope from this PR, and would not prevent the bug from reappearing.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newError could be dropped sure, and withStackTrace should probably be renamed, but that is out of scope from this PR, and would not prevent the bug from reappearing.

I would say adding the additionalStackDepthToSkip parameter doesn't prevent the bug from happening either. Someone new to the errors package creating a new error creating function would probably just copy what is written in another function and possibly have the wrong value for the parameter.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is really worth the effort, it is an extremely localized point of contention and I feel like the level of push back received is grossly disproportionate to any downsides of the approach taken in the PR

The thing is that although this is localized for the purpose of the discussion, I see the outcome of discussions like this to have a very broad impact as it will influence our (at least mine) approach to other implementations. I also think about new devs looking at this package (since it's small and approachable) to observe our coding style and them seeing either the opinionated approach or the flexible one and then copying the observed approach for their own work. This is why I'm pushing back and I'm not convinced by your explanation (doesn't mean it's not valid). If someone else prefers your approach, I'll be happy to approve the PR :)

Copy link
Member

@jsimnz jsimnz Dec 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gone through this thread a few times now, read the old code, the new PR code, the references pkg/errors code Fred linked, a few other go error pkgs that include stack behavior, etc.

In the majority of cases I prefer "explicit" convention that is documented, compared to complete freedom, however this package has a fairly small surface area, and although the cognitive load is certainly higher to understand the skipStack parameters, any time you touch the code in this package, it's a manageable price, for the clarity it provides. So I think its OK to keep as is.

However, I also think that its within scope to drop newError and rely soley on withStackTrace. Lastly I'd like to see better docstring on withStackTrace to make it very clear the current behavior of depthToSkip and how to use it as a caller (even if it may seem obvious)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can get on board with that change.

Extra nitpick: depthToSkip would be better than additionalStackDepthToSkip if you don't mind changing it.

Copy link
Contributor Author

@AndrewSisley AndrewSisley Dec 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll add some docs.

depthToSkip feels like it looses a lot of info, but I guess additional is internal to withStackTrace and Stack is somewhat implicit given the function name - will change.

  • doc
  • rename

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also removed newError

stackBuffer := make([]uintptr, MaxStackDepth)

// Skip the first X frames as they are part of this library (and dependencies) and are
// best hidden.
length := runtime.Callers(4, stackBuffer[:])
// best hidden, also account for any parent calls within this library.
const depthFromHereToSkip int = 2
length := runtime.Callers(depthFromHereToSkip+additionalStackDepthToSkip, stackBuffer[:])
stack := stackBuffer[:length]
stackText := toString(stack)

Expand Down
70 changes: 24 additions & 46 deletions errors/errors_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,27 +35,20 @@ func TestErrorIsDefraError(t *testing.T) {
}

func TestErrorWithStack(t *testing.T) {
err := errors.New("gndjdhs")
errorMessage := "gndjdhs"
err := errors.New(errorMessage)

errWithStach := WithStack(err)
errWithStack := WithStack(err)

result := fmt.Sprintf("%+v", errWithStach)
result := fmt.Sprintf("%+v", errWithStack)

/*
The Go test flag `-race` messes with the stacktrace causing this function's frame to be ommited from
the stacktrace, as our CI runs with the `-race` flag, these assertions need to be disabled.
// Assert that the first line starts with the error message and contains this [test] function's stacktrace-line -
// including file, line number, and function reference. An exact string match should not be used as the stacktrace
// is machine dependent.
assert.Regexp(t, fmt.Sprintf("^%s\\. Stack: .*\\/defradb\\/errors\\/errors_test\\.go:[0-9]+ \\([a-zA-Z0-9]*\\)", errorMessage), result)

// Assert that the first line starts with the error message and contains this [test] function's stacktrace-line -
// including file, line number, and function reference. An exact string match should not be used as the stacktrace
// is machine dependent.
assert.Regexp(t, fmt.Sprintf("^%s\\. Stack: .*\\/defradb\\/errors\\/errors_test\\.go:[0-9]+ \\([a-zA-Z0-9]*\\)", errorMessage), result)
// Assert that the error contains this function's name, and a print-out of the generating line.
assert.Regexp(t, "TestErrorFmtvWithStacktrace: err := Error\\(errorMessage\\)", result)
*/

// As noted above, we cannot assert that this function's stack frame is included in the trace,
// however we should still assert that the error message is present.
assert.Regexp(t, fmt.Sprintf("^%s\\. Stack: ", err.Error()), result)
// Assert that the error contains this function's name, and a print-out of the generating line.
assert.Regexp(t, "TestErrorWithStack: errWithStack := WithStack\\(err\\)", result)

// Assert that the next line of the stacktrace is also present.
assert.Regexp(t, ".*\\/testing/testing.go:[0-9]+ \\([a-zA-Z0-9]*\\)", result)
Expand Down Expand Up @@ -146,23 +139,16 @@ func TestErrorFmtvWithStacktrace(t *testing.T) {
const errorMessage string = "gndjdhs"

err := New(errorMessage)
result := fmt.Sprintf("%+v", err)

/*
The Go test flag `-race` messes with the stacktrace causing this function's frame to be ommited from
the stacktrace, as our CI runs with the `-race` flag, these assertions need to be disabled.
result := fmt.Sprintf("%+v", err)

// Assert that the first line starts with the error message and contains this [test] function's stacktrace-line -
// including file, line number, and function reference. An exact string match should not be used as the stacktrace
// is machine dependent.
assert.Regexp(t, fmt.Sprintf("^%s\\. Stack: .*\\/defradb\\/errors\\/errors_test\\.go:[0-9]+ \\([a-zA-Z0-9]*\\)", errorMessage), result)
// Assert that the error contains this function's name, and a print-out of the generating line.
assert.Regexp(t, "TestErrorFmtvWithStacktrace: err := Error\\(errorMessage\\)", result)
*/
// Assert that the first line starts with the error message and contains this [test] function's stacktrace-line -
// including file, line number, and function reference. An exact string match should not be used as the stacktrace
// is machine dependent.
assert.Regexp(t, fmt.Sprintf("^%s\\. Stack: .*\\/defradb\\/errors\\/errors_test\\.go:[0-9]+ \\([a-zA-Z0-9]*\\)", errorMessage), result)

// As noted above, we cannot assert that this function's stack frame is included in the trace,
// however we should still assert that the error message is present.
assert.Regexp(t, fmt.Sprintf("^%s\\. Stack: ", errorMessage), result)
// Assert that the error contains this function's name, and a print-out of the generating line.
assert.Regexp(t, "TestErrorFmtvWithStacktrace: err := New\\(errorMessage\\)", result)

// Assert that the next line of the stacktrace is also present.
assert.Regexp(t, ".*\\/testing/testing.go:[0-9]+ \\([a-zA-Z0-9]*\\)", result)
Expand All @@ -174,21 +160,13 @@ func TestErrorFmtvWithStacktraceAndKvps(t *testing.T) {
err := New(errorMessage, NewKV("Kv1", 1), NewKV("Kv2", "2"))
result := fmt.Sprintf("%+v", err)

/*
The Go test flag `-race` messes with the stacktrace causing this function's frame to be ommited from
the stacktrace, as our CI runs with the `-race` flag, these assertions need to be disabled.

// Assert that the first line starts with the error message and contains this [test] function's stacktrace-line -
// including file, line number, and function reference. An exact string match should not be used as the stacktrace
// is machine dependent.
assert.Regexp(t, fmt.Sprintf("^%s\\. Kv1: 1, Kv2: 2\\. Stack: .*\\/defradb\\/errors\\/errors_test\\.go:[0-9]+ \\([a-zA-Z0-9]*\\)", errorMessage), result)
// Assert that the error contains this function's name, and a print-out of the generating line.
assert.Regexp(t, "TestErrorFmtvWithStacktraceAndKvps: err := Error\\(errorMessage\\)", result)
*/

// As noted above, we cannot assert that this function's stack frame is included in the trace,
// however we should still assert that the error message is present.
assert.Regexp(t, fmt.Sprintf("^%s\\. Kv1: 1, Kv2: 2\\. Stack: ", errorMessage), result)
// Assert that the first line starts with the error message and contains this [test] function's stacktrace-line -
// including file, line number, and function reference. An exact string match should not be used as the stacktrace
// is machine dependent.
assert.Regexp(t, fmt.Sprintf("^%s\\. Kv1: 1, Kv2: 2\\. Stack: .*\\/defradb\\/errors\\/errors_test\\.go:[0-9]+ \\([a-zA-Z0-9]*\\)", errorMessage), result)

// Assert that the error contains this function's name, and a print-out of the generating line.
assert.Regexp(t, "TestErrorFmtvWithStacktraceAndKvps: err := New\\(errorMessage, NewKV\\(\"Kv1\", 1\\), NewKV\\(\"Kv2\", \"2\"\\)\\)", result)

// Assert that the next line of the stacktrace is also present.
assert.Regexp(t, ".*\\/testing/testing.go:[0-9]+ \\([a-zA-Z0-9]*\\)", result)
Expand Down