util/log: prevent ReportPanic() from swallowing panics in some cases #52201
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
Prior to this patch, if `ReportPanic()` was called and the panic object ultimately discarded (ie. caught) during tests not otherwise using `TestLogScope`, the panic object would be lost. This was causing panic objects to disappear quite a lot in fact, because the test runner *also* catches panics and fails to report them adequately in some cases (e.g. when the panic occurs during a `stress` run). To alleviate the situation, this patch removes some logic previously present in the code that was attempting (and failing) to remove duplicate panic prints. That logic was misdesigned to start with anyway (by me), because it was working under the assumption that `ReportPanic()` was *only* called for uncaught panics, and another function `RecoverAndReportNonfatalPanic()` just before that was blatantly violating that assumption. Removing the logic causes reportable panics to always be reported in logs and on stderr, regardless of whether it's caught or not. This provides more guarantees that the panic object will be seen, at the expense of having a duplicate print in some edge cases. Release note: None
knz
force-pushed
the
20200731-double-panic
branch
from
72e090b to
e9a159b
Compare
andreimatei
reviewed
Aug 3, 2020
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @andreimatei and @irfansharif)
|
thanks! bors r=andreimatei |
|
Build succeeded: |
knz
added a commit
to knz/cockroach
that referenced
this pull request
Aug 25, 2020
This flake was introduced in a recent bump of `cockroachdb/errors`, combined with cockroachdb#52201. Release note: None
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Accompanies #52200 (but is an independent change)
Prior to this patch, if
ReportPanic()was called and the panicobject ultimately discarded (ie. caught) during tests that were not otherwise using
TestLogScope, the panic object would be lost. I believe this was directly noticed by @andreimatei prior.I also believe this was causing panic objects to disappear quite a lot in fact,
because the test runner also catches panics and fails to report
them adequately in some cases (e.g. when the panic occurs during a
stressrun).To alleviate the situation, this patch removes some logic previously
present in the code that was attempting (and failing) to remove
duplicate panic prints. That logic was misdesigned to start with anyway
(by me), because it was working under the assumption that
ReportPanic()was only called for uncaught panics, and anotherfunction
RecoverAndReportNonfatalPanic()just before that wasblatantly violating that assumption.
Removing the logic causes reportable panics to always be reported in
logs and on stderr, regardless of whether it's caught or not. This
provides more guarantees that the panic object will be seen, at the
expense of having a duplicate print in some edge cases.
Release note: None