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

Handle more panics #3577

Merged
merged 6 commits into from
Nov 5, 2024
Merged

Handle more panics #3577

merged 6 commits into from
Nov 5, 2024

Conversation

MDrakos
Copy link
Member

@MDrakos MDrakos commented Nov 1, 2024

BugDX-3136 Rollbar: Could not report messages as CheckMessages return an error: Error sending messages request:

@github-actions github-actions bot changed the base branch from master to version/0-48-0-RC1 November 1, 2024 20:02
@MDrakos MDrakos marked this pull request as ready for review November 1, 2024 20:55
Comment on lines 24 to 29
wrappedFn := func() (interface{}, error) {
defer func() {
panics.LogAndPanic(recover(), debug.Stack())
}()
return pollFunc()
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Panics can occur in the goroutine where the poll func is started. If we don't want this automatic wrapping we can update each caller of this library to manually inject it into their pollfunc.

@MDrakos MDrakos requested a review from Naatan November 1, 2024 20:56
cmd/state-svc/internal/messages/messages.go Outdated Show resolved Hide resolved
cmd/state-svc/internal/messages/messages.go Outdated Show resolved Hide resolved
Comment on lines 24 to 29
wrappedFn := func() (interface{}, error) {
defer func() {
panics.LogAndPanic(recover(), debug.Stack())
}()
return pollFunc()
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to implement this as part of the pollFunc that we pass. Reason being that if we catch it here the stacktrace won't point to what caused the panic.

@MDrakos MDrakos requested a review from Naatan November 5, 2024 22:38
@MDrakos MDrakos merged commit 9eb410e into version/0-48-0-RC1 Nov 5, 2024
8 checks passed
@MDrakos MDrakos deleted the DX-3136 branch November 5, 2024 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants