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.

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

gracefully handle elided frames #120

Merged
merged 6 commits into from
Apr 13, 2024
Merged

Conversation

drshriveer
Copy link
Contributor

When parsing stacks we frequently encounter elided frames. The actual stack line looks something like ...23 frames elided..., which is passed to parseFuncName, which of course is not parsable as a function name. As a result, whenever this happens we get a useless panic like the following:

panic: Failed to parse stack trace: parse function: no function found: "...23 frames elided..."
goroutine 1 [running]:
go.uber.org/goleak/internal/stack.getStackBuffer(0x1)
	/home/runner/go/pkg/mod/go.uber.org/goleak@v1.3.0/internal/stack/stacks.go:240 +0x65
go.uber.org/goleak/internal/stack.getStacks(0x1)
	/home/runner/go/pkg/mod/go.uber.org/goleak@v1.3.0/internal/stack/stacks.go:84 +0x3b
go.uber.org/goleak/internal/stack.All(...)
	/home/runner/go/pkg/mod/go.uber.org/goleak@v1.3.0/internal/stack/stacks.go:229
go.uber.org/goleak.Find({0xc0003ae880, 0x4, 0x4})
	/home/runner/go/pkg/mod/go.uber.org/goleak@v1.3.0/leaks.go:65 +0x1ea

This PR gracefully handles elided frames to prevent the panic.

@CLAassistant
Copy link

CLAassistant commented Mar 27, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@sywhang sywhang left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for the contribution!

internal/stack/stacks.go Outdated Show resolved Hide resolved
"goroutine 1 [running]:",
"example.com/foo/bar.baz()",
" example.com/foo/bar.go:123",
"...3 frames elided...",
Copy link
Collaborator

Choose a reason for hiding this comment

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

In addition to this fixed data test case, can we add or update a test that creates a goroutine with a deep stack that leads to elided frames, and verify that? That way, we'll have coverage that will detect if the elided message has changed.

This is a real concern, when the eliding logic was originally added, it used "stack frames omitted". Given that it has changed, we should have a test that will detect changes by relying on the real runtime logic.

@drshriveer
Copy link
Contributor Author

@prashantv addressed both comments. If everything looks good, please help merge it in :) - I don't have the necessary permisisons.

internal/stack/stacks_test.go Outdated Show resolved Hide resolved
@prashantv prashantv merged commit b62053b into uber-go:master Apr 13, 2024
2 of 5 checks passed
prashantv added a commit that referenced this pull request Apr 13, 2024
Follow-up to #120, I accidentally merged the PR without waiting for all
tests to run, and missed that 1.20 tests are failing.

Lint is failing because of how we skip golangci-lint run in the action
and use the Makefile. Use the same logic as zap to skip lint.

Tests are failing on 1.20 as stack elision doesn't kick in for the test.
From 1.21 onwards, it does kick in. Rather than try to make the test
trigger stack elision on 1.20, let's bump the versions to 1.21 / 1.22.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants