Default reconnect strategy uses exponential backoff and jitter #2736
Conversation
Both are recommended parts of client reconnect strategies to prevent thundering herd problems when many clients lose their connection at once (for example, during a Redis upgrade).
qsymmachus
changed the title
|
Hi @qsymmachus, thanks for contributing! |
docs/client-configuration.md
Outdated
| @@ -13,7 +13,7 @@ | |||
| | socket.keepAlive | `true` | Toggle [`keep-alive`](https://nodejs.org/api/net.html#socketsetkeepaliveenable-initialdelay) functionality | | |||
| | socket.keepAliveInitialDelay | `5000` | If set to a positive number, it sets the initial delay before the first keepalive probe is sent on an idle socket | | |||
| | socket.tls | | See explanation and examples [below](#TLS) | | |||
| | socket.reconnectStrategy | `retries => Math.min(retries * 50, 500)` | A function containing the [Reconnect Strategy](#reconnect-strategy) logic | | |||
| | socket.reconnectStrategy | `((retries^2) * 50 ms) + 0-200 ms` | A function containing the [Reconnect Strategy](#reconnect-strategy) logic | | |||
There was a problem hiding this comment.
the ^ operator is "Bitwise XOR" in JavaScript, not power.. we need to find a better way to document this..
There was a problem hiding this comment.
I went with a plain English explanation of the strategy in 38eb603, rather than try to figure out how to pseudo-code this in a legible way.
packages/client/lib/client/socket.ts
Outdated
| @@ -82,6 +82,15 @@ export default class RedisSocket extends EventEmitter { | |||
| } | |||
|
|
|||
| #createReconnectStrategy(options?: RedisSocketOptions): ReconnectStrategyFunction { | |||
| const defaultStrategy = (retries: number) => { | |||
There was a problem hiding this comment.
I think this should be moved to a #defaultReconnectStrategy function and used from lines 109 and 114
|
@leibale I responded to your latest feedback; sorry for the delay! |
|
@leibale bumping this one! Thanks for taking another look. |
|
Docs look good to me. |
|
I'll leave the that @leibale as he is the primary owner. I just pop in and do the occasional review. ;) |
|
@qsymmachus done! thanks for taking the time to contribute, and sorry for the delays.. |
Description
The current default reconnect strategy increases delay linearly with no jitter. This can lead to a thundering herd problem if many clients lose their connection at once, which is common during events like a Redis upgrade.
A reconnect strategy that uses exponential backoff plus a random jitter helps mitigate this risk and is recommended in many best practices documents for Redis clients, so I propose making this the default.
The default strategy I implemented will do the following:
(n^2) * 50 mswherenis the current retry count, with a maximum value of 2000 ms.The millisecond values I choose are somewhat arbitrary, and I'm happy to change them if we think they're not right.
Checklist
npm testpass with this change (including linting)?reconnectStrategystill pass. We have no tests for the default strategy, nor am I sure if and how we'd want to test it. Any thoughts on this?