fix: Correct errors.WithStack behaviour #984
Conversation
AndrewSisley
added
bug
Different entry points result in a different depth - this results in a failing test with or without a `race` flag. Also, when compiling with optimizations enabled the compiler would inline the `New` function resulting in an incorrect result (the --race flag must do this).
AndrewSisley
force-pushed
the
sisley/fix/I983-with-stack
branch
from
33558b7
to
5ae9133
Compare
Codecov Report
@@ Coverage Diff @@
## develop #984 +/- ##
========================================
Coverage 57.77% 57.77%
========================================
Files 172 172
Lines 19455 19455
========================================
Hits 11241 11241
Misses 7225 7225
Partials 989 989
|
They arent inlined now as they do stuff in them, and that seems to be all the go compiler cares about, but this should help prevent the introduction of new bugs should this change
errors/errors.go
Outdated
| // 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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
newErrorcould be dropped sure, andwithStackTraceshould 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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
I can get on board with that change.
Extra nitpick: depthToSkip would be better than additionalStackDepthToSkip if you don't mind changing it.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I've also removed newError
errors/errors.go
Outdated
| // 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 { |
There was a problem hiding this comment.
todo: If newError will be kept, would be nice to have a line of documentation for the additionalStackDepthToSkip parameter.
AndrewSisley
force-pushed
the
sisley/fix/I983-with-stack
branch
from
15380b0
to
f35f269
Compare
* Correct typo * Remove --race ommision from TestWithStack * Correctly handle varying stack depth generated within this library Different entry points result in a different depth - this results in a failing test with or without a `race` flag. Also, when compiling with optimizations enabled the compiler would inline the `New` function resulting in an incorrect result (the --race flag must do this). * Include ommited test assertions * Block inlining of other stack-sensitive functions They arent inlined now as they do stuff in them, and that seems to be all the go compiler cares about, but this should help prevent the introduction of new bugs should this change
Relevant issue(s)
Resolves #983
Description
Were two bugs here - firstly the number of stack frames skipped was incorrect for
WithStackregardless to compile flags. Secondly when compiling with optimisations theNewfunction was inlined resulting in a different stack depth - the tests omitted the checks that caught this as the original author (me) falsely assumed it was theraceflag doing weird stuff - this will likely have been visible in production builds since it went in (unlikeWithStackwhich is newer).