You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(common/countdown): stop draining the timer channel on Reset - #3034
startTimer drains timer.C when Stop reports false, then re-arms the timer. That was the documented idiom before Go 1.23, when timer channels were buffered: a value the select had not taken yet was still in the channel, so the drain returned and the Reset that followed re-armed the timer. Go 1.23 made the channel unbuffered, and the same receive now waits for a send that never arrives once the value has been taken, leaving startTimer wedged with every later Reset and StopTimer queued behind it.
Reset stops the timer and discards a pending value by itself, so the drain is dead weight with a live failure mode. The automatic re-arm after a timeout is the only thing keeping it out of reach today.
Summary
startTimer
select
case <-timer.C
go OnTimeoutFn(...)
timer.Reset(duration) # automatic re-arm
case <-t.resetc
duration = GetTimeoutDuration(...)
- if !timer.Stop() {- <-timer.C # blocks once the select took the value- }
timer.Reset(duration) # stops and re-arms in one step
Evidence
Before:go test ./common/countdown/ -run TestResetAfterCallbackReturns -timeout 30s on this branch's base (4ac2cc42cd) with the automatic re-arm removed:
--- FAIL: TestResetAfterCallbackReturns (1.05s)
countdown_reset_test.go:62: callback did not fire after the reset: the reset path is blocked
After: the same mutation on this branch:
ok github.com/XinFinOrg/XDPoSChain/common/countdown 0.104s
The re-arm is what keeps the drain out of reach, so removing it is the smallest edit that reaches the branch. A Reset issued from a callback after the timer fired is the production shape, and TestCountdownShouldAvoidDeadlock already drives it.
Timer semantics on Go 1.25, standalone: time.NewTimer(50ms) → receive → Stop() reports false → a second receive blocks, because the channel is unbuffered and the value is gone.
New test: -count=100 10.07s, -race -count=30 4.05s, -race -cpu=1 -count=20 3.03s, all green.
Full suite on 9e9eda27e5: make all, make generate (No stale files detected), make tidy (No untidy module files detected), make lint (0 issues), go vet ./..., make test — 130 packages ok, common/countdown ok 38.1s.
Merge Danger
Door: two-way
One drain removed; reverting restores it.
Blast Radius: consensus
common/countdown is the XDPoS v2 timeout timer, and every engine Reset goes through this branch. The behaviour is unchanged on Go 1.23+: Reset already stopped the timer and discarded a pending value, so the drain only ever ran on a stale value that the next Reset overwrote anyway.
Types of changes
What types of changes does your code introduce to XDC network? Put an ✅ in the boxes that apply
build: Changes that affect the build system or external dependencies
ci: Changes to CI configuration files and scripts
chore: Changes that don't change source code or tests
docs: Documentation only changes
feat: A new feature
fix: A bug fix
perf: A code change that improves performance
refactor: A code change that neither fixes a bug nor adds a feature
revert: Revert something
style: Changes that do not affect the meaning of the code
test: Adding missing tests or correcting existing tests
Impacted Components
Which parts of the codebase does this PR touch? Put an ✅ in the boxes that apply
Consensus
Account
Network
Geth
Smart Contract
External components
Not sure (Please specify below)
Checklist
Put an ✅ in the boxes once you have confirmed below actions (or provide reasons on not doing so) that
This PR has sufficient test coverage (unit/integration test) OR I have provided reason in the PR description for not having test coverage
Tested on a private network from the genesis block and monitored the chain operating correctly for multiple epochs.
Provide an end-to-end test plan in the PR description on how to manually test it on the devnet/testnet.
Tested the backwards compatibility.
Tested with XDC nodes running this version co-exist with those running the previous version.
Relevant documentation has been updated as part of this PR
N/A
The four unchecked item are node-level: the reset path is covered by TestResetAfterCallbackReturns and the existing package tests, and on Go 1.23+ the change removes a drain that never ran on a live timer, so there is no devnet behaviour to observe, no cross-version difference to check, and no documentation to update.
Summary by CodeRabbit
Bug Fixes
Countdown timers can now be reset reliably after their callback has run, without the reset operation becoming stuck. The timer is re-armed for the updated duration, and the next countdown callback can run as expected. This improves the responsiveness and reliability of countdowns when they are reset after completing a countdown.
Reviewing files that changed from the base of the PR and between fa615de and ffab945.
Files selected for processing: 2
common/countdown/countdown.go
common/countdown/countdown_reset_test.go
No actionable comments were generated in the recent review.
Walkthrough
The countdown reset path now calls timer.Reset without first stopping and draining the timer channel. A new test checks that reset returns after a callback and that another callback follows.
Changes: Countdown reset
Layer / File(s)
Summary
Reset behavior and regression test common/countdown/countdown.go, common/countdown/countdown_reset_test.go
The reset path no longer stops the timer and conditionally drains its channel before calling timer.Reset. The test checks that reset returns after a callback and that another callback follows.
The countdown reset change is ready to merge after normal checks.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title clearly describes the main change to the countdown timer reset path.
Description check
The description includes the required sections, test evidence, impacted component, and reasons for unchecked checklist items. The reported before-and-after test used a mutation that removed automatic …
Docstring Coverage
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Description check
Explanation
The description includes the required sections, test evidence, impacted component, and reasons for unchecked checklist items. The reported before-and-after test used a mutation that removed automatic re-arming.
Full details: Docstring Coverage
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 2 files.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Reviewing files that changed from the base of the PR and between e4caf93 and fa615de.
Files selected for processing: 2
common/countdown/countdown.go
common/countdown/countdown_reset_test.go
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
startTimer drained timer.C whenever Stop reported false, which was the
documented way to re-arm a timer before Go 1.23: the channel was buffered,
so a value the select had not taken yet was still there to take. Go 1.23
made the channel unbuffered, and the same receive now waits for a send that
never comes once the value has been taken, wedging the countdown goroutine
with every later Reset and StopTimer queued behind it.
Reset stops the timer and discards a pending value on its own, so the drain
is dead weight with a live failure mode: the automatic re-arm after a
timeout is the only thing that keeps it out of reach today, and skipping
that re-arm turns it into a permanent block.
countdown_reset_test.go covers the path: a Reset issued from a callback
after the timer fired has to return and leave the timer armed for the next
callback. With the re-arm removed, that test fails inside its one second
bound on the old code and passes on this one.
This file contains hidden or 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
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.
Proposed changes
startTimerdrainstimer.CwhenStopreports false, then re-arms the timer. That was the documented idiom before Go 1.23, when timer channels were buffered: a value theselecthad not taken yet was still in the channel, so the drain returned and theResetthat followed re-armed the timer. Go 1.23 made the channel unbuffered, and the same receive now waits for a send that never arrives once the value has been taken, leavingstartTimerwedged with every laterResetandStopTimerqueued behind it.Resetstops the timer and discards a pending value by itself, so the drain is dead weight with a live failure mode. The automatic re-arm after a timeout is the only thing keeping it out of reach today.Summary
startTimer select case <-timer.C go OnTimeoutFn(...) timer.Reset(duration) # automatic re-arm case <-t.resetc duration = GetTimeoutDuration(...) - if !timer.Stop() { - <-timer.C # blocks once the select took the value - } timer.Reset(duration) # stops and re-arms in one stepEvidence
Before:
go test ./common/countdown/ -run TestResetAfterCallbackReturns -timeout 30son this branch's base (4ac2cc42cd) with the automatic re-arm removed:After: the same mutation on this branch:
The re-arm is what keeps the drain out of reach, so removing it is the smallest edit that reaches the branch. A
Resetissued from a callback after the timer fired is the production shape, andTestCountdownShouldAvoidDeadlockalready drives it.Timer semantics on Go 1.25, standalone:
time.NewTimer(50ms)→ receive →Stop()reportsfalse→ a second receive blocks, because the channel is unbuffered and the value is gone.New test:
-count=10010.07s,-race -count=304.05s,-race -cpu=1 -count=203.03s, all green.Full suite on
9e9eda27e5:make all,make generate(No stale files detected),make tidy(No untidy module files detected),make lint(0 issues),go vet ./...,make test— 130 packages ok,common/countdownok 38.1s.Merge Danger
Door: two-way
One drain removed; reverting restores it.
Blast Radius: consensus
common/countdownis the XDPoS v2 timeout timer, and every engineResetgoes through this branch. The behaviour is unchanged on Go 1.23+:Resetalready stopped the timer and discarded a pending value, so the drain only ever ran on a stale value that the nextResetoverwrote anyway.Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which parts of the codebase does this PR touch?
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) thatThe four unchecked item are node-level: the reset path is covered by
TestResetAfterCallbackReturnsand the existing package tests, and on Go 1.23+ the change removes a drain that never ran on a live timer, so there is no devnet behaviour to observe, no cross-version difference to check, and no documentation to update.Summary by CodeRabbit