-
Notifications
You must be signed in to change notification settings - Fork 479
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
catchup: do not loop forever if there is no peers #6037
catchup: do not loop forever if there is no peers #6037
Conversation
* Add context cancellation check to fetchRound peers retrieval loop * This prevented some e2e tests to finish when a other nodes quit but the last node fell into catchup mode
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6037 +/- ##
==========================================
- Coverage 55.87% 55.87% -0.01%
==========================================
Files 482 482
Lines 68571 68576 +5
==========================================
+ Hits 38317 38318 +1
+ Misses 27652 27649 -3
- Partials 2602 2609 +7 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ... I see you are just following the pattern below, but there is a slightly shorter 1-line check for ctx cancelled/timeout besides Done():
if err := s.ctx.Err(); err != nil {
logging.Base().Debugf("ctx is done: %v", err)
return
}
as per https://pkg.go.dev/context#Context
// If Done is not yet closed, Err returns nil.
// If Done is closed, Err returns a non-nil error explaining why:
// Canceled if the context was canceled
// or DeadlineExceeded if the context's deadline passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @cce's comment, but that's not a blocker
Summary
TestCatchupOverGossip
from tests: preserve logs on LibGoalFixture failure #6030Diagnosed with debug logging in
TestCatchupOverGossip
: catchup service attempted to stop but never finished while keep logging entries below until it got killed.Test Plan
Existing tests