Repository navigation
fix(rpc): stop draining the ping timer channel on Reset - #3035
Merged
gzliudan merged 1 commit intoNov 12, 2026
Merged
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 30 minutes. View limit details
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. |
gzliudan
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410,
liam-lai and
wanwiset25
November 6, 2026 17:26
gzliudan
force-pushed
the
fix-websocket-ping-drain
branch
from
November 7, 2026 14:51
f016616 to
3c7e396
Compare
anunay-xin
approved these changes
Nov 9, 2026
gzliudan
force-pushed
the
fix-websocket-ping-drain
branch
from
November 9, 2026 08:35
3c7e396 to
2b9d9f0
Compare
The pingLoop reset path drained pingTimer.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 pingLoop goroutine with every later pingReset 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 an idle ping is the only thing that keeps it out of reach today, and skipping that re-arm turns it into a permanent block. websocket_pingloop_test.go covers the path: a pingReset arriving after the timer fired has to return and leave the timer armed for the next ping. With the re-arm removed, that test fails inside its two second bound on the old code and passes on this one. wsPingInterval becomes a variable so the test can drive the timer at test speed.
gzliudan
force-pushed
the
fix-websocket-ping-drain
branch
from
November 9, 2026 10:36
2b9d9f0 to
1a5d17e
Compare
wanwiset25
approved these changes
Nov 12, 2026
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
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.
Proposed changes
pingLoopdrainspingTimer.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, leavingpingLoopwedged with every laterpingResetqueued 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 an idle ping is the only thing keeping it out of reach today. This mirrors #3034, which removes the same drain incommon/countdown.Summary
pingLoop select case <-wc.closed() return case <-wc.pingReset - if !pingTimer.Stop() { - <-pingTimer.C # blocks once the select took the value - } pingTimer.Reset(wsPingInterval) case <-pingTimer.C send ping frame pingTimer.Reset(wsPingInterval) # automatic re-arm case <-wc.pongReceived clear read deadlineEvidence
Before: with the automatic re-arm removed on this branch's base (
4ac2cc42cd) plus the const-to-variable test seam, the new test: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
pingResetarriving after the timer fired is the production shape: every successfulwriteJSONsends one.New test matrices:
-count=1004.14s,-race -count=302.29s,-race -cpu=1 -count=201.86s, all green.Full suite on
f016616fd3:make all,make generate(No stale files detected),make tidy(No untidy module files detected),make lint(0 issues),go vet ./...,make test— all packages ok.Merge Danger
Door: two-way
One drain removed and one constant turned into a variable with the same value; reverting restores both.
Blast Radius: rpc
rpc/websocket.gois the WebSocket transport for JSON-RPC, and every connection'spingLoopgoes 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 value that the nextResetoverwrote anyway. The worst historical failure mode was a wedgedpingLoopon a single connection.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 unchecked items are node-level: on Go 1.23+ the change removes a drain that never ran on a live timer and turns a constant into a variable with the same value, so there is no devnet behaviour to observe, no cross-version difference to check, and no documentation to update.