Use runtime.CallersFrames to create StackFrames #113
Conversation
The output of Stack() isn't stable across versions of Go and shouldn't be relied on. That makes the tests fail against Go 1.10, 1.11, and 1.12 (possibly earlier too). Instead, the stack frames in Error should be verified against the stack frames supplied by the runtime. Although the stack frames may vary from runtime impl to impl and version to version, we are comparing like for like so the tests shouldn't break.
The stack frame information in Error were got by various functions in the runtime pkg. This is explicitly discouraged in the [docs](https://golang.org/pkg/runtime/#Callers) which instead suggest to use runtime.CallersFrames to translate PCs to symbolic info.
This should be sufficient and robust against differing number of frames between runtimes
Not introduced until Go 1.9
|
Hi @aduong, thanks for the PR. Due to the scope of this it'll take us a while to get it reviewed. I can't give you an ETA at this point, but we have a lot of other higher priority things going on at the moment. Thanks for your patience! |
errors/stackframe.go
Outdated
|
|
||
| // NewStackFrameFromRuntime populates a stack frame object from a runtime.Frame object. | ||
| func NewStackFrameFromRuntime(frame runtime.Frame) StackFrame { | ||
| pkg, name := packageAndName(frame.Func) |
There was a problem hiding this comment.
It seems that frame.Func can still be nil sometimes. Might be worth attempting to salvage something from frame.Function in that case.
There was a problem hiding this comment.
Yes, that seems reasonable. I've pushed a commit that does this.
@bengourley, thanks for the prompt response. If it helps, I can split this change set so that the failing tests are separate set. As it turns out, the error_test suite was already being skipped because it was platform-dependent. Part of this change set makes those tests platform independent. |
|
Sadly, it doesn't seem the team had the capacity to review this and this is probably too outdated. My team has been able to work around the issue. Perhaps this has been fixed in later versions. |
|
Is there any chance of reviving this PR? We're running into an issue where we have an incorrect stack trace being generated for a panic that looks to be corrected by this change. It's very difficult to debug further into the runtime, but seems like common sense to follow the guidance of the Go documentation:
We may consider forking our own client to get this fix in, but if it this could be merged here that would be much preferable. |
|
Hey @mathewbyrne, seen your message on #114 so we'll continue the conversation there. |
Goal
Stack traces from
errors.Errorare incomplete and miss functions. In particular, inlined functions are missing.Additionally, the tests for
errorswere failing.Also fixes #44
Design
The change is to user
runtime.CallersFramesto translate program counters to symbolic information as recommended by the golang docs.Changeset
Added
errors.NewStackFrameFromRuntimeRemoved
None
Changed
errors.Error.StackFrameTests
Unit tests were fixed to fail for the right reasons (stack frame content, not formatting). All unit tests were run and passed.
Outstanding Questions
errors.StackFrame.Func()still relies onruntime.FuncForPCwhich is less than ideal. Within the library, it is primarily used on constructor ofStackFrames. One way to give the rightFuncwould be to save theFuncon theStackFrameitself on construction from aruntime.Frame.Review
For the submitter, initial self-review:
For the pull request reviewer(s), this changeset has been reviewed for: