fix: debounce for ensurePublisherConnected. #967
Conversation
📝 WalkthroughWalkthroughIntroduces connection caching for publisher connections in the Engine class. A new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lib/src/core/engine.dart`:
- Around line 506-510: ensurePublisherConnected currently caches the future in
_publisherConnectionFuture but never clears it if _publisherEnsureConnected()
throws or times out, causing permanent failure; modify ensurePublisherConnected
to assign _publisherConnectionFuture = _publisherEnsureConnected() as now, then
await it inside a try/catch, and in the catch block reset
_publisherConnectionFuture to null before rethrowing the error so subsequent
calls can retry; reference functions/fields: ensurePublisherConnected,
_publisherConnectionFuture, and _publisherEnsureConnected.
- Around line 1148-1150: Cached publisher connection future
_publisherConnectionFuture can remain completed after restartConnection()
causing ensurePublisherConnected() to skip renegotiation; when
disposing/recreating the publisher inside restartConnection() clear/reset
_publisherConnectionFuture (set to null or a fresh completer) so
ensurePublisherConnected() will recreate the connection for the new publisher
instance; update restartConnection() teardown path where the old publisher is
disposed to reset _publisherConnectionFuture before new publisher creation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/src/core/engine.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build for Flutter Android
- GitHub Check: Build for Flutter Web WASM
- GitHub Check: Build for Flutter Linux
- GitHub Check: Build for Flutter Windows
- GitHub Check: Build for Flutter macOS
- GitHub Check: Build for Flutter iOS
- GitHub Check: Build for Flutter Web
🔇 Additional comments (3)
lib/src/core/engine.dart (3)
168-168: LGTM: cached in-flight publisher connection state.The field addition is clear and well-scoped for debouncing.
411-423: LGTM: avoid sending before publisher is connected.Good guard to ensure the publisher transport is ready when subscriber is primary.
643-650: LGTM: cache reset on disconnected/failed/closed states.This keeps the memoized future aligned with connection lifecycle.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| @internal | ||
| Future<void> ensurePublisherConnected() async { | ||
| _publisherConnectionFuture ??= _publisherEnsureConnected(); | ||
| await _publisherConnectionFuture; | ||
| } |
There was a problem hiding this comment.
Reset cached future on failure to allow retries.
If _publisherEnsureConnected() times out or throws, _publisherConnectionFuture stays in a failed state and all future calls immediately rethrow. Clear the cache on error so subsequent attempts can retry.
🛠️ Proposed fix
`@internal`
Future<void> ensurePublisherConnected() async {
_publisherConnectionFuture ??= _publisherEnsureConnected();
- await _publisherConnectionFuture;
+ try {
+ await _publisherConnectionFuture;
+ } catch (_) {
+ _publisherConnectionFuture = null;
+ rethrow;
+ }
}🤖 Prompt for AI Agents
In `@lib/src/core/engine.dart` around lines 506 - 510, ensurePublisherConnected
currently caches the future in _publisherConnectionFuture but never clears it if
_publisherEnsureConnected() throws or times out, causing permanent failure;
modify ensurePublisherConnected to assign _publisherConnectionFuture =
_publisherEnsureConnected() as now, then await it inside a try/catch, and in the
catch block reset _publisherConnectionFuture to null before rethrowing the error
so subsequent calls can retry; reference functions/fields:
ensurePublisherConnected, _publisherConnectionFuture, and
_publisherEnsureConnected.
There was a problem hiding this comment.
I think coderabbitai is right, if _publisherEnsureConnected() can throw, _publisherConnectionFuture() will cache the state and rethrow, can you address it ?
There was a problem hiding this comment.
@xianshijing-lk, you're absolutely right! The current implementation will cache the failed future state, preventing any retries. I'll prepare the fix for you.
Task: Fix ensurePublisherConnected to reset cache on failure
- Wrap the await in a try-catch block
- Reset
_publisherConnectionFutureto null before rethrowing - This allows subsequent calls to retry the connection
💡 For code changes across multiple files, use @coderabbitai plan in an issue comment
| if (_hasPublished) { | ||
| await _publisherEnsureConnected(); | ||
| await ensurePublisherConnected(); | ||
| } |
There was a problem hiding this comment.
Clear cached future on restart to avoid stale success.
restartConnection() disposes and recreates the publisher. If _publisherConnectionFuture was completed in the prior session, ensurePublisherConnected() may no-op and skip negotiation for the new publisher (especially in subscriber-primary + non-fast-publish). Reset the cache when tearing down the old publisher.
🛠️ Proposed fix
await publisher?.dispose();
publisher = null;
+ _publisherConnectionFuture = null;🤖 Prompt for AI Agents
In `@lib/src/core/engine.dart` around lines 1148 - 1150, Cached publisher
connection future _publisherConnectionFuture can remain completed after
restartConnection() causing ensurePublisherConnected() to skip renegotiation;
when disposing/recreating the publisher inside restartConnection() clear/reset
_publisherConnectionFuture (set to null or a fresh completer) so
ensurePublisherConnected() will recreate the connection for the new publisher
instance; update restartConnection() teardown path where the old publisher is
disposed to reset _publisherConnectionFuture before new publisher creation.
xianshijing-lk
left a comment
xianshijing-lk
left a comment
There was a problem hiding this comment.
please address the ai bot comments, I found it pretty useful.
| @internal | ||
| Future<void> ensurePublisherConnected() async { | ||
| _publisherConnectionFuture ??= _publisherEnsureConnected(); | ||
| await _publisherConnectionFuture; | ||
| } |
There was a problem hiding this comment.
I think coderabbitai is right, if _publisherEnsureConnected() can throw, _publisherConnectionFuture() will cache the state and rethrow, can you address it ?
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.