Repository navigation
fix(sio-client): prevent the reconnection delay from dropping to zero - #5582
Merged
darrachequesne merged 2 commits intoNov 5, 2026
Merged
Conversation
The exponential part of the backoff (reconnectionDelay * 2^attempts) overflows to Infinity after 1015 attempts, which is about 85 minutes of failed reconnections with the default options. The jitter then computes Infinity - Infinity = NaN, and NaN | 0 = 0, so about half of the next reconnection attempts happen without any delay. The value is now capped at Number.MAX_VALUE before the jitter is applied, so the delay keeps being reconnectionDelayMax, as it was before the overflow. Co-Authored-By: Claude Opus 5.5 <[email protected]>
darrachequesne
left a comment
darrachequesne
left a comment
Member
There was a problem hiding this comment.
Thanks a lot for your pull request. Please see my comments below.
| var ms = this.ms * Math.pow(this.factor, this.attempts++); | ||
| // after many attempts, the value overflows to Infinity, and the jitter | ||
| // below would then compute Infinity - Infinity = NaN (so a delay of 0) | ||
| ms = Math.min(ms, Number.MAX_VALUE); |
Member
There was a problem hiding this comment.
If jitter is enabled, I think that this might still overflow:
const ms = Number.MAX_VALUE;
const deviation = Math.floor(rand * jitter * ms);
ms + deviation; // InfinityHow about something like:
Backoff.prototype.duration = function(){
var ms = this.ms * Math.pow(this.factor, this.attempts++);
- if (this.jitter) {
+ if (this.jitter && isFinite(ms)) {
var rand = Math.random();
var deviation = Math.floor(rand * this.jitter * ms);
ms = (Math.floor(rand * 10) & 1) == 0 ? ms - deviation : ms + deviation;
}
return Math.min(ms, this.max) | 0;
};This way:
Infinityis properly handled- behavior of always returning
maxwhenattemptsis sufficiently big is preserved
What do you think?
Contributor
Author
There was a problem hiding this comment.
Good catch, that reads better too. Switched to if (this.jitter && isFinite(ms)) in 272c275. With attempts = 1015 it now always returns 5000, and normal delays are unchanged.
Suggested in review: this also covers ms + deviation overflowing near Number.MAX_VALUE, and keeps returning the max delay after many attempts. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Member
|
Nice, thanks 👍 |
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.
The kind of change this PR does introduce
Current behavior
The exponential part of the reconnection backoff (
reconnectionDelay * 2^attempts, inlib/contrib/backo2.ts) overflows toInfinityat attempt 1015 with the default options (1000 * 2^1015 > Number.MAX_VALUE), which is after about 85 minutes of failed reconnection attempts (reconnectionAttemptsdefaults toInfinity).From there, when the jitter subtracts the deviation, it computes
Infinity - Infinity = NaN, andMath.min(NaN, max) | 0is0. So about half of the following reconnection attempts are scheduled with no delay instead ofreconnectionDelayMax.Reproduction:
New behavior
The exponential part is capped at
Number.MAX_VALUEbefore the jitter is applied. The delay then stays atreconnectionDelayMax, as it already does once the backoff has reached it (from the 5th attempt with the default options). Delays computed before the overflow are unchanged.A test is added in
test/connection.ts.Other information (e.g. related issues)
I did not find an existing issue or pull request about this.
I used Claude Code to write this change.