Fix false positive race tests by gating all R/W access to timers behind synchronization #875
Conversation
…er to elimiate false positives in race tests.
WalkthroughThe health checking mechanism in the server protocol has been enhanced with the addition of a new method, Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/daemons/server/types/health_checker.go (2 hunks)
Additional comments: 2
protocol/daemons/server/types/health_checker.go (2)
115-127: The implementation of
InitializePollingcorrectly ensures that the timer is only initialized if it has not been previously set, which should prevent the race condition false positives as intended by the PR.221-221: The call to
InitializePollingwithinStartNewHealthCheckercorrectly schedules the first poll using the new synchronized method, aligning with the PR's objective to synchronize timer initialization.
| @@ -112,6 +112,20 @@ func (u *healthCheckerMutableState) SchedulePoll(nextPollDelay time.Duration) { | |||
| u.timer.Reset(nextPollDelay) | |||
There was a problem hiding this comment.
The SchedulePoll method assumes that the timer is already initialized when calling Reset. It might be prudent to add a check to ensure that the timer is not nil before calling Reset to avoid a potential nil pointer dereference if SchedulePoll is called before InitializePolling.
func (u *healthCheckerMutableState) SchedulePoll(nextPollDelay time.Duration) {
u.lock.Lock()
defer u.lock.Unlock()
// Don't schedule a poll if the health checker has been stopped.
if u.stopped {
return
}
+ // Ensure the timer is initialized before resetting it.
+ if u.timer == nil {
+ return
+ }
u.timer.Reset(nextPollDelay)
}Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| u.timer.Reset(nextPollDelay) | |
| func (u *healthCheckerMutableState) SchedulePoll(nextPollDelay time.Duration) { | |
| u.lock.Lock() | |
| defer u.lock.Unlock() | |
| // Don't schedule a poll if the health checker has been stopped. | |
| if u.stopped { | |
| return | |
| } | |
| // Ensure the timer is initialized before resetting it. | |
| if u.timer == nil { | |
| return | |
| } | |
| u.timer.Reset(nextPollDelay) | |
| } |
| // The first poll is scheduled after a custom delay to allow the service to initialize. | ||
| u.timer = time.AfterFunc(firstPollDelay, pollFunc) | ||
| } | ||
|
|
||
| // Stop stops the health checker. This method is synchronized. | ||
| func (u *healthCheckerMutableState) Stop() { |
There was a problem hiding this comment.
The Stop method should check if the timer is not nil before calling Stop on it to prevent a potential nil pointer dereference if Stop is called before the timer is initialized.
func (u *healthCheckerMutableState) Stop() {
u.lock.Lock()
defer u.lock.Unlock()
// Don't stop the health checker if it has already been stopped.
if u.stopped {
return
}
+ // Ensure the timer is initialized before stopping it.
+ if u.timer == nil {
+ return
+ }
u.timer.Stop()
u.stopped = true
}Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (u *healthCheckerMutableState) Stop() { | |
| func (u *healthCheckerMutableState) Stop() { | |
| u.lock.Lock() | |
| defer u.lock.Unlock() | |
| // Don't stop the health checker if it has already been stopped. | |
| if u.stopped { | |
| return | |
| } | |
| // Ensure the timer is initialized before stopping it. | |
| if u.timer == nil { | |
| return | |
| } | |
| u.timer.Stop() | |
| u.stopped = true | |
| } |
Changelist
Race tests are detecting potential conflict in the initialization of the health checker mutable state timer, and updates to the timer after polling starts. This race is logically impossible, but to prevent the false positive I moved the initialization of the timer to a method where it isgated behind the lock on the mutable state object.
Test Plan
Existing tests should continue to pass + no more race failures.
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.