Add Liveness Mechanism in WatchDocument #941
Conversation
WalkthroughThis PR introduces two new optional properties— Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant T as Timer
participant S as Server/Event Stream
participant R as Reconnector
C->>T: Start idle timer (idleStreamTimeout)
loop Watch Loop
S-->>C: Emit event
C->>T: Reset timer
end
alt Idle timeout exceeded
T-->>C: Trigger timeout
C->>R: Abort connection & schedule reconnect (exponential backoff up to maxIdleStreamTimeout)
end
Assessment against linked issues
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
packages/sdk/src/client/client.tsOops! Something went wrong! :( ESLint: 8.19.0 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/packages/sdk".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "packages/sdk/.eslintrc.js » ../../.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. ✨ Finishing Touches
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 your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
krapie
force-pushed
the
add-liveness-in-watchdocument
branch
from
67aa970 to
8e0d499
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #941 +/- ##
==========================================
+ Coverage 78.50% 78.52% +0.02%
==========================================
Files 63 63
Lines 5419 5453 +34
Branches 997 1005 +8
==========================================
+ Hits 4254 4282 +28
- Misses 874 878 +4
- Partials 291 293 +2 ☔ View full report in Codecov by Sentry. |
|
I changed it to ready temporarily to activate CodeRabbit reviews. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/sdk/src/client/client.ts (1)
160-164: 🛠️ Refactor suggestionInconsistent default value in comment and code
The comment for
reconnectionStreamTimeoutstates that the default value is30000ms, but inDefaultClientOptions, it is set to60000ms. Please update the comment or the default value to ensure consistency.Apply this diff to correct the comment:
/** * `reconnectionStreamTimeout` is the timeout of the reconnection stream. If * the stream is disconnected, the client waits for the timeout to reconnect - * the stream. The default value is `30000`(ms). + * the stream. The default value is `60000`(ms). */
🧹 Nitpick comments (2)
packages/sdk/src/client/client.ts (2)
161-162: Improve wording in comment for clarityThe sentence "the client waits for the timeout to reconnect the stream" could be clearer. Consider rephrasing it to "the client waits for the timeout before attempting to reconnect the stream" for better clarity.
Apply this diff to rephrase the comment:
* the stream is disconnected, the client waits for the timeout to reconnect - * the stream. + * the stream before attempting to reconnect.
887-891: Extract magic number into a constantThe maximum timeout value
300000ms (5 minutes) is hardcoded in the exponential backoff calculation. Consider extracting it into a named constant or making it configurable for better readability and flexibility.Apply this diff to define a constant for the maximum timeout:
+ const MAX_RECONNECTION_TIMEOUT = 300000; // 5 minutesAnd update the calculation:
currentTimeout = Math.min( reconnectionStreamTimeout * Math.pow(2, consecutiveTimeouts - 1), - 300000, + MAX_RECONNECTION_TIMEOUT, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/sdk/public/index.html(1 hunks)packages/sdk/src/client/client.ts(8 hunks)
🔇 Additional comments (1)
packages/sdk/public/index.html (1)
323-323: Verify the server port in client instantiationThe client is being instantiated with the URL
'http://localhost:80', changed from'http://localhost:8080'. Ensure that the Yorkie server is correctly configured to listen on port 80. If the server is still running on port 8080, this change might cause connection issues.
krapie
force-pushed
the
add-liveness-in-watchdocument
branch
from
bddd46b to
3bd9eb4
Compare
krapie
force-pushed
the
add-liveness-in-watchdocument
branch
from
3bd9eb4 to
ed17da5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/sdk/src/client/client.ts (1)
914-914: Consider using a more specific type for the timer.Based on the past review comments,
NodeJS.Timeoutcaused errors in other packages. Consider creating a custom type alias or usingReturnType<typeof setTimeout>instead ofany.- let idleTimer: any; + let idleTimer: ReturnType<typeof setTimeout>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/sdk/src/client/client.ts(11 hunks)
🔇 Additional comments (4)
packages/sdk/src/client/client.ts (4)
158-171: Extract default timeout values as constants.Consider extracting the default timeout values as named constants at the top of the file for better maintainability and reusability.
+const DEFAULT_IDLE_STREAM_TIMEOUT = 60000; +const DEFAULT_MAX_IDLE_STREAM_TIMEOUT = Infinity; const DefaultClientOptions = { syncLoopDuration: 50, retrySyncLoopDelay: 1000, reconnectStreamDelay: 1000, - idleStreamTimeout: 60000, - maxIdleStreamTimeout: Infinity, + idleStreamTimeout: DEFAULT_IDLE_STREAM_TIMEOUT, + maxIdleStreamTimeout: DEFAULT_MAX_IDLE_STREAM_TIMEOUT, };Also applies to: 181-182
246-249: LGTM!The initialization of new timeout properties follows the existing pattern and correctly uses nullish coalescing for default values.
941-950: LGTM! Well-implemented exponential backoff with jitter.The exponential backoff implementation with jitter is robust and follows best practices:
- Base delay increases exponentially
- Jitter adds randomization to prevent thundering herd
- Maximum timeout is enforced
972-974: LGTM! Proper cleanup of timer resources.The timer cleanup in the error handling path ensures no memory leaks.
Co-authored-by: changhui lee <[email protected]>
krapie
force-pushed
the
add-liveness-in-watchdocument
branch
from
527f085 to
34554e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/sdk/src/client/client.ts (4)
158-171: Add unit information to the documentation.The documentation should specify that the timeout values are in milliseconds.
Apply this diff to improve the documentation:
/** - * `idleStreamTimeout` is the timeout of the reconnection stream. If - * the stream is disconnected, the client waits for the timeout to reconnect - * the stream. The default value is `60000`(ms). + * `idleStreamTimeout` is the initial timeout in milliseconds for detecting idle + * connections. When no events are received within this duration, the client + * attempts to reconnect. The default value is `60000` milliseconds (1 minute). */ idleStreamTimeout?: number; /** - * `maxIdleStreamTimeout` is the maximum timeout of the reconnection stream. - * If the stream is disconnected, the client waits for the timeout to reconnect - * the stream. The default value is `Infinity`(ms). + * `maxIdleStreamTimeout` is the maximum timeout in milliseconds that the + * exponential backoff can reach. This caps the delay between reconnection + * attempts. The default value is `Infinity` milliseconds. */ maxIdleStreamTimeout?: number;
876-878: Reset backoff attempts on successful connection.The
backoffAttemptscounter should be reset when the connection is successfully established, not just when events are received.Add the reset in the initialization case:
if (resp.body.case === 'initialization') { + backoffAttempts = 0; + currentTimeout = idleStreamTimeout; resolve([stream, ac]); }
941-951: Extract backoff calculation into a utility function.The exponential backoff calculation could be extracted into a reusable utility function, making it easier to test and maintain.
Consider moving this logic to a separate utility function:
// In a separate utility file export function calculateExponentialBackoff( baseTimeout: number, maxTimeout: number, attempts: number ): number { const exponentialDelay = baseTimeout * Math.pow(2, attempts - 1); const jitteredDelay = baseTimeout + Math.random() * (exponentialDelay - baseTimeout); return Math.min(jitteredDelay, maxTimeout); } // In the watch loop const updateTimeoutWithBackoff = (): number => { backoffAttempts++; currentTimeout = calculateExponentialBackoff( idleStreamTimeout, maxIdleStreamTimeout, backoffAttempts ); return currentTimeout; };
914-928: Move timer management functions outside the promise callback.The timer management functions could be moved outside the promise callback to improve code organization and reusability.
Consider refactoring the timer management into a class:
class IdleTimer { private timer?: ReturnType<typeof setTimeout>; private currentTimeout: number; private backoffAttempts: number = 0; constructor( private readonly baseTimeout: number, private readonly maxTimeout: number, private readonly onTimeout: () => void ) { this.currentTimeout = baseTimeout; } schedule(): void { this.clear(); this.timer = setTimeout(() => { this.onTimeout(); this.updateTimeoutWithBackoff(); this.schedule(); }, this.currentTimeout); } reset(): void { this.clear(); if (this.backoffAttempts > 0) { this.backoffAttempts = 0; this.currentTimeout = this.baseTimeout; } this.schedule(); } clear(): void { if (this.timer) { clearTimeout(this.timer); } } private updateTimeoutWithBackoff(): void { this.backoffAttempts++; this.currentTimeout = calculateExponentialBackoff( this.baseTimeout, this.maxTimeout, this.backoffAttempts ); } }Also applies to: 929-940
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/sdk/src/client/client.ts(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (18.x)
🔇 Additional comments (1)
packages/sdk/src/client/client.ts (1)
213-214: LGTM!The client properties and constructor initialization are implemented correctly, with proper handling of default values.
Also applies to: 246-249
| idleStreamTimeout: 60000, | ||
| maxIdleStreamTimeout: Infinity, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider setting a reasonable maximum timeout.
Using Infinity as the maximum timeout could lead to very long reconnection delays. Consider setting a more reasonable maximum value, such as 5 minutes (300000ms).
Apply this diff to set a reasonable maximum timeout:
- maxIdleStreamTimeout: Infinity,
+ maxIdleStreamTimeout: 300000, // 5 minutes📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| idleStreamTimeout: 60000, | |
| maxIdleStreamTimeout: Infinity, | |
| idleStreamTimeout: 60000, | |
| maxIdleStreamTimeout: 300000, // 5 minutes |
What this PR does / why we need it?
Adds a liveness mechanism to the
WatchDocumentfeature. If no events are received for a document within 60 seconds, the client will attempt to re-establish the connection. This enhancement addresses disconnections caused by split-brain issues in the server's long-lived connection, reducing unnecessary reconnections during collaborative editing.The proposed logic uses a 60-second connection re-establishment counter that resets upon receiving an event. If the counter reaches zero, a reconnection occurs. Additionally, the counter will adjust based on event frequency, allowing quicker reconnections when active and longer intervals during inactivity (exponential backoff + jitter).
Key benefits of this feature:
Any background context you want to provide?
This enhancement aims to improve connection management and user experience by minimizing disconnections during collaborative editing while ensuring timely reconnections when necessary.
What are the relevant tickets?
Fixes #940
Checklist
Summary by CodeRabbit