fix: UnSubscribe removes entry from map regardless of socket state + add removeSubscriptions, clearAllSubscriptions and resetInstance utilities#1142
Open
assajeeb wants to merge 4 commits into
Conversation
Fix unSubscribe (remove from map even when socket is closed)
Add clearAllSubscriptions() inside LiveQueryClient
Add resetInstance() static method inside LiveQueryClient
Future<void> disposeRoom() async {
// ... close streams, cancel timers, clear queues ...
final subs = [subscription, subscription2, subscription3];
subscription = null;
subscription2 = null;
subscription3 = null;
// unSubscribe all first (Edit 1 ensures this works even if socket closed)
for (final sub in subs) {
if (sub != null) {
try { LiveQueryClient.instance.unSubscribe(sub); } catch (_) {}
}
}
// Full reset — map cleared, socket closed, singleton nulled
await LiveQueryClient.resetInstance();
print("disposeRoom done");
}
add remove single subscription remove
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
Bot
changed the title
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
📝 WalkthroughWalkthroughThis PR extends the LiveQueryClient with batch subscription removal and clear-all utilities, adds a static resetInstance() to fully reset the singleton, and refactors unSubscribe to always clean up local subscription state while only sending the websocket unsubscribe when a channel exists. ChangesSubscription lifecycle management
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/dart/lib/src/network/parse_live_query.dart`:
- Around line 235-241: resetInstance currently leaves the old
LiveQueryReconnectingController able to reconnect because it only calls
disconnect(userInitialized: false); update resetInstance to explicitly
stop/disable the reconnect loop on the old instance's controller (e.g., call the
LiveQueryReconnectingController's disable/stop/dispose method) before or
immediately after calling disconnect and before clearing _instance so the old
instance cannot respond to lifecycle/connectivity events; reference the existing
variable (existing) and its reconnect controller (e.g.,
existing.reconnectingController) and ensure the controller is put into a
non-reconnecting state.
- Around line 302-308: The unSubscribe snippet currently only checks that
_channel is non-null but may still be closed/closing; update the logic in the
unSubscribe handling around _channel (referencing _channel, unsubscribeMessage,
_debug, _printConstLiveQuery) to ensure the socket is open before calling
channel.sink.add: first check an "open" indicator (e.g. channel.closeCode ==
null / no closeCode or other available open-state property on the concrete
channel implementation) and only call sink.add when open, and additionally wrap
the send in a try/catch to swallow/log errors instead of letting sink.add throw
on closing/closed sockets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03f5e004-1ed2-4621-9315-d224c2387a94
📒 Files selected for processing (1)
packages/dart/lib/src/network/parse_live_query.dart
Comment on lines
+235
to
+241
| static Future<void> resetInstance() async { | ||
| final existing = _instance; | ||
| if (existing != null) { | ||
| existing.clearAllSubscriptions(); | ||
| try { | ||
| await existing.disconnect(userInitialized: false); | ||
| } catch (_) {} |
There was a problem hiding this comment.
resetInstance() does not fully disable the old reconnect loop
Line 240 uses disconnect(userInitialized: false), which leaves the old LiveQueryReconnectingController in reconnect-capable state. After reset, the old instance can still reconnect from lifecycle/connectivity events.
Suggested fix
static Future<void> resetInstance() async {
final existing = _instance;
if (existing != null) {
existing.clearAllSubscriptions();
try {
- await existing.disconnect(userInitialized: false);
+ await existing.disconnect(userInitialized: true);
} catch (_) {}
}
_instance = null;
_requestIdCount = 1;
}📝 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.
Suggested change
| static Future<void> resetInstance() async { | |
| final existing = _instance; | |
| if (existing != null) { | |
| existing.clearAllSubscriptions(); | |
| try { | |
| await existing.disconnect(userInitialized: false); | |
| } catch (_) {} | |
| static Future<void> resetInstance() async { | |
| final existing = _instance; | |
| if (existing != null) { | |
| existing.clearAllSubscriptions(); | |
| try { | |
| await existing.disconnect(userInitialized: true); | |
| } catch (_) {} | |
| } | |
| _instance = null; | |
| _requestIdCount = 1; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/dart/lib/src/network/parse_live_query.dart` around lines 235 - 241,
resetInstance currently leaves the old LiveQueryReconnectingController able to
reconnect because it only calls disconnect(userInitialized: false); update
resetInstance to explicitly stop/disable the reconnect loop on the old
instance's controller (e.g., call the LiveQueryReconnectingController's
disable/stop/dispose method) before or immediately after calling disconnect and
before clearing _instance so the old instance cannot respond to
lifecycle/connectivity events; reference the existing variable (existing) and
its reconnect controller (e.g., existing.reconnectingController) and ensure the
controller is put into a non-reconnecting state.
Comment on lines
+302
to
308
| WebSocketChannel? channel = _channel; | ||
| if (channel != null) { | ||
| if (_debug) { | ||
| print('$_printConstLiveQuery: UnsubscribeMessage: $unsubscribeMessage'); | ||
| } | ||
| channel.sink.add(jsonEncode(unsubscribeMessage)); | ||
| } |
There was a problem hiding this comment.
unSubscribe still sends on non-open channels
Line 303 only checks non-null channel; it does not ensure the socket is open. In disconnected/closing states this can throw on sink.add(...).
Suggested fix
WebSocketChannel? channel = _channel;
- if (channel != null) {
+ final webSocket = _webSocket;
+ if (channel != null &&
+ webSocket != null &&
+ webSocket.readyState == parse_web_socket.WebSocket.open) {
if (_debug) {
print('$_printConstLiveQuery: UnsubscribeMessage: $unsubscribeMessage');
}
channel.sink.add(jsonEncode(unsubscribeMessage));
}📝 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.
Suggested change
| WebSocketChannel? channel = _channel; | |
| if (channel != null) { | |
| if (_debug) { | |
| print('$_printConstLiveQuery: UnsubscribeMessage: $unsubscribeMessage'); | |
| } | |
| channel.sink.add(jsonEncode(unsubscribeMessage)); | |
| } | |
| WebSocketChannel? channel = _channel; | |
| final webSocket = _webSocket; | |
| if (channel != null && | |
| webSocket != null && | |
| webSocket.readyState == parse_web_socket.WebSocket.open) { | |
| if (_debug) { | |
| print('$_printConstLiveQuery: UnsubscribeMessage: $unsubscribeMessage'); | |
| } | |
| channel.sink.add(jsonEncode(unsubscribeMessage)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/dart/lib/src/network/parse_live_query.dart` around lines 302 - 308,
The unSubscribe snippet currently only checks that _channel is non-null but may
still be closed/closing; update the logic in the unSubscribe handling around
_channel (referencing _channel, unsubscribeMessage, _debug,
_printConstLiveQuery) to ensure the socket is open before calling
channel.sink.add: first check an "open" indicator (e.g. channel.closeCode ==
null / no closeCode or other available open-state property on the concrete
channel implementation) and only call sink.add when open, and additionally wrap
the send in a try/catch to swallow/log errors instead of letting sink.add throw
on closing/closed sockets.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dart/lib/src/network/parse_live_query.dart (1)
370-373:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
onErrorhandler still uses directsink.addinstead of_safeAddEvent.The PR objectives state that
onErrorshould use_safeAddEvent, but this handler still calls_clientEventStreamController.sink.add(...)directly. This can throw if the controller is closed, which is exactly what_safeAddEventwas introduced to prevent.🐛 Proposed fix
onError: (Object error) { - _clientEventStreamController.sink.add( - LiveQueryClientEvent.disconnected, - ); + _safeAddEvent(LiveQueryClientEvent.disconnected); if (_debug) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dart/lib/src/network/parse_live_query.dart` around lines 370 - 373, Replace the direct call to _clientEventStreamController.sink.add in the onError handler with a call to _safeAddEvent so we don't throw when the controller is closed: locate the onError closure that currently does _clientEventStreamController.sink.add(LiveQueryClientEvent.disconnected) and change it to call _safeAddEvent(LiveQueryClientEvent.disconnected) (keeping the same event value) so the helper handles closed-controller safety.
🧹 Nitpick comments (4)
packages/dart/lib/src/network/parse_live_query.dart (4)
491-492: 💤 Low valueFix indentation.
This line should be indented to match the method body level (6 spaces within
_handleMessage).🧹 Suggested fix
-_safeAddEvent(LiveQueryClientEvent.connected); + _safeAddEvent(LiveQueryClientEvent.connected);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dart/lib/src/network/parse_live_query.dart` around lines 491 - 492, The line calling _safeAddEvent(LiveQueryClientEvent.connected); followed by return is mis-indented inside the _handleMessage method; adjust the indentation of that statement block to the same level as the rest of the method body (6 spaces inside _handleMessage) so it aligns with surrounding statements and maintains consistent formatting for the _handleMessage function.
228-236: 💤 Low valueFix indentation for class method.
Same indentation fix needed here.
🧹 Suggested fix
-/// Removes all subscriptions from the internal map. -/// Call before disconnect when leaving a live room. - -void clearAllSubscriptions() { - _requestSubscription.values.toList().forEach((sub) { - sub._enabled = false; - }); - _requestSubscription.clear(); -} + /// Removes all subscriptions from the internal map. + /// Call before disconnect when leaving a live room. + void clearAllSubscriptions() { + for (final sub in _requestSubscription.values) { + sub._enabled = false; + } + _requestSubscription.clear(); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dart/lib/src/network/parse_live_query.dart` around lines 228 - 236, The method clearAllSubscriptions is misindented and should be nested correctly inside its class; move/adjust its indentation to match other class methods so it aligns with the class member scope (ensure clearAllSubscriptions, its body referencing _requestSubscription, _requestSubscription.values.toList().forEach(...) and sub._enabled are indented consistently with surrounding methods).
204-208: 💤 Low valueFix indentation in
_safeAddEvent.The method body is not properly indented within the function block.
🧹 Suggested fix
- void _safeAddEvent(LiveQueryClientEvent event) { - if (!_clientEventStreamController.isClosed) { - _clientEventStreamController.sink.add(event); - } -} + void _safeAddEvent(LiveQueryClientEvent event) { + if (!_clientEventStreamController.isClosed) { + _clientEventStreamController.sink.add(event); + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dart/lib/src/network/parse_live_query.dart` around lines 204 - 208, The _safeAddEvent method's body is mis-indented; adjust indentation so the if block and its inner call are indented one level inside the method (use the project's Dart indentation style, e.g., two spaces) and keep the closing brace aligned with the method declaration. Locate function _safeAddEvent and ensure _clientEventStreamController and .sink.add(event) lines are properly nested under the if statement and the method's braces.
218-226: 💤 Low valueFix indentation for class method.
The method declaration should be indented to align with other class methods (2 spaces).
🧹 Suggested fix
-/// Removes specific subscriptions from the map by requestId. -/// Use this instead of clearAllSubscriptions() when other subscriptions -/// (banners, notifications) must stay alive. -void removeSubscriptions(List<Subscription> subs) { - for (final sub in subs) { - sub._enabled = false; - _requestSubscription.remove(sub.requestId); + /// Removes specific subscriptions from the map by requestId. + /// Use this instead of clearAllSubscriptions() when other subscriptions + /// (banners, notifications) must stay alive. + void removeSubscriptions(List<Subscription> subs) { + for (final sub in subs) { + sub._enabled = false; + _requestSubscription.remove(sub.requestId); + } } -}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dart/lib/src/network/parse_live_query.dart` around lines 218 - 226, The removeSubscriptions method is mis-indented; adjust its declaration and body to use 2-space indentation to match other class methods. Locate the removeSubscriptions(List<Subscription> subs) method and align the comment, method signature, and body so the leading spaces are two, keeping the logic that sets sub._enabled = false and calls _requestSubscription.remove(sub.requestId) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/dart/lib/src/network/parse_live_query.dart`:
- Around line 370-373: Replace the direct call to
_clientEventStreamController.sink.add in the onError handler with a call to
_safeAddEvent so we don't throw when the controller is closed: locate the
onError closure that currently does
_clientEventStreamController.sink.add(LiveQueryClientEvent.disconnected) and
change it to call _safeAddEvent(LiveQueryClientEvent.disconnected) (keeping the
same event value) so the helper handles closed-controller safety.
---
Nitpick comments:
In `@packages/dart/lib/src/network/parse_live_query.dart`:
- Around line 491-492: The line calling
_safeAddEvent(LiveQueryClientEvent.connected); followed by return is
mis-indented inside the _handleMessage method; adjust the indentation of that
statement block to the same level as the rest of the method body (6 spaces
inside _handleMessage) so it aligns with surrounding statements and maintains
consistent formatting for the _handleMessage function.
- Around line 228-236: The method clearAllSubscriptions is misindented and
should be nested correctly inside its class; move/adjust its indentation to
match other class methods so it aligns with the class member scope (ensure
clearAllSubscriptions, its body referencing _requestSubscription,
_requestSubscription.values.toList().forEach(...) and sub._enabled are indented
consistently with surrounding methods).
- Around line 204-208: The _safeAddEvent method's body is mis-indented; adjust
indentation so the if block and its inner call are indented one level inside the
method (use the project's Dart indentation style, e.g., two spaces) and keep the
closing brace aligned with the method declaration. Locate function _safeAddEvent
and ensure _clientEventStreamController and .sink.add(event) lines are properly
nested under the if statement and the method's braces.
- Around line 218-226: The removeSubscriptions method is mis-indented; adjust
its declaration and body to use 2-space indentation to match other class
methods. Locate the removeSubscriptions(List<Subscription> subs) method and
align the comment, method signature, and body so the leading spaces are two,
keeping the logic that sets sub._enabled = false and calls
_requestSubscription.remove(sub.requestId) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43bf64df-2901-41f6-ada1-ee1e449cea49
📒 Files selected for processing (1)
packages/dart/lib/src/network/parse_live_query.dart
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
1 participant
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.
Problem
When
unSubscribe()is called after the WebSocket is already closed,the
_requestSubscriptionmap entry is never removed because theremoval was gated inside
if (channel != null).This causes ghost subscriptions to accumulate in the map. On every
WebSocket reconnect, the
connectedhandler iterates the map andre-sends subscribe messages for all entries — including stale ones
from previous sessions that should have been cleaned up.
Root Cause
A common teardown pattern is:
In this case
unSubscribeworks correctly. But ifdisconnect()iscalled first — or if the socket drops before
unSubscribeis called —the channel is already null and the map entry is never removed.
Changes
1. Fix
unSubscribe— always remove from mapThe unsubscribe op is only sent over the socket if the channel is open
(correct behaviour), but
_enabled = falseand map removal now alwaysexecute regardless of socket state.
2. Add
removeSubscriptions(List<Subscription> subs)Removes a targeted set of subscriptions from the internal map without
affecting others. Useful when multiple independent features share the
same singleton client and only one group needs to be torn down.
3. Add
clearAllSubscriptions()Disables and removes all entries from
_requestSubscriptionwithoutclosing the socket.
4. Add
resetInstance()Full singleton reset for cases where a completely fresh client is
needed. Clears all subscriptions, closes the socket, and nulls the
singleton so the next
LiveQueryClient()call creates a new instance.Uses
userInitialized: falseso the reconnecting controller remainsactive after reset.
Impact
unSubscribecall signature unchangedlifecycle without workarounds
Summary by CodeRabbit
New Features
Improvements