leaktest: exclude long running logging goroutines #93989
Merged
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
The `leaktest` package detects potential goroutine leaks by snapshotting the set of goroutines running when `leaktest.AfterTest(t)` is called, returning a closure, and comparing the set of goroutines when the closure is called (typically `defer`'d). A race condition was uncovered in cockroachdb#93849 whereby logging-related goroutines that are scheduled by an `init` function in `pkg/util/logging` can sometimes be spawned _after_ the `AfterTest` function is run. When the test completes and the closure is run, the test fails due to a difference in the before / after goroutine snapshots. This mode of failure is deemed to be a false-positive. The intention of the logging goroutines are that they live for the duration of the process. However, exactly _when_ the goroutines scheduled in the `init` functions actually start run, and hence show up in the goroutine snapshots, is non-deterministic. Exclude the logging goroutines from the `leaktest` checks to reduce the flakiness of tests. Closes cockroachdb#93849. Release note: None.
nicktrav
requested review from
srosenberg,
a team and
smg260
and removed request for
a team
|
This change is |
srosenberg
approved these changes
Dec 21, 2022
srosenberg
approved these changes
Dec 21, 2022
There was a problem hiding this comment.
Thanks for adding the exclusions; no more future flakes owing to these false positives.
Btw, the CI failure was already addressed; hopefully, it should pass on the next run; slack thread: https://cockroachlabs.slack.com/archives/C016CAD2HQ8/p1671566665663939?thread_ts=1671566463.649659&cid=C016CAD2HQ8
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @smg260)
|
TFTR! bors r=srosenberg |
|
Build succeeded: |
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.
The
leaktestpackage detects potential goroutine leaks by snapshotting the set of goroutines running whenleaktest.AfterTest(t)is called, returning a closure, and comparing the set of goroutines when the closure is called (typicallydefer'd).A race condition was uncovered in #93849 whereby logging-related goroutines that are scheduled by an
initfunction inpkg/util/loggingcan sometimes be spawned after theAfterTestfunction is run. When the test completes and the closure is run, the test fails due to a difference in the before / after goroutine snapshots.This mode of failure is deemed to be a false-positive. The intention of the logging goroutines are that they live for the duration of the process. However, exactly when the goroutines scheduled in the
initfunctions actually start run, and hence show up in the goroutine snapshots, is non-deterministic.Exclude the logging goroutines from the
leaktestchecks to reduce the flakiness of tests.Closes #93849.
Release note: None.
Epic: CRDB-20293