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

storage: replace a few panics with log.Fatal #16482

Merged

Conversation

petermattis
Copy link
Collaborator

panic causes stacks to unwind and defers to be invoked (possibly
releasing locks) until eventually something either handles the
panic (unlikely) or we reach the runtime and it kills the process. While
the process is dying, other goroutines are still running which may
encounter invalid states due to the panic. These invalid states can
produce fatal errors of there own which seem impossible and are
difficult to debug. For fatal errors we should use log.Fatal{,f} instead
which will kill the process before any locks are released.

See #16479

panic causes stacks to unwind and defers to be invoked (possibly
releasing locks) until eventually something either handles the
panic (unlikely) or we reach the runtime and it kills the process. While
the process is dying, other goroutines are still running which may
encounter invalid states due to the panic. These invalid states can
produce fatal errors of there own which seem impossible and are
difficult to debug. For fatal errors we should use log.Fatal{,f} instead
which will kill the process before any locks are released.

See cockroachdb#16479
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@petermattis petermattis requested a review from bdarnell June 13, 2017 00:10
@petermattis
Copy link
Collaborator Author

I didn't convert all of the panics, only a few choice ones in Store and Replica.

@bdarnell
Copy link
Contributor

:lgtm:


Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@petermattis petermattis merged commit bd6326a into cockroachdb:master Jun 13, 2017
@petermattis petermattis deleted the pmattis/prefer-log-fatal-to-panic branch June 13, 2017 12:27
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.

3 participants