Skip to content

fix(sql_connect): queries are not single instanced for execute #18500

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
aashishpatil-g wants to merge 1 commit into
base: main
Choose a base branch
from
+37 −1
Open

fix(sql_connect): queries are not single instanced for execute #18500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/firebase_data_connect/firebase_data_connect/lib/src/core/ref.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,7 @@ class QueryRef<Data, Variables> extends OperationRef<Data, Variables> {
_serverStreamSubscription?.cancel();
_serverStreamSubscription = null;
_serverStream = null;
_streamController = null;

@dconeybe dconeybe Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent QueryRef identity and potential duplicate server streams.

When all subscribers to a query cancel, the QueryRef is currently removed from trackedQueries (via the stream controller's onCancel). If query() is subsequently called again for the same operation, a new QueryRef instance will be created. If the developer still holds the old QueryRef and resubscribes to it, both the old and new instances will be active, leading to multiple active server streams for the same logical query.

To fix this and maintain a single canonical instance, QueryManager should use WeakReference to track queries, allowing them to be reused if they still exist in memory, while avoiding leaks.

Please apply the following changes to QueryManager (not shown in this diff):

  1. Change trackedQueries type to use WeakReference:
final Map<String, WeakReference<QueryRef<dynamic, dynamic>>> trackedQueries = {};
  1. Update addQuery to store WeakReference and NOT remove it on cancel:
  StreamController<QueryResult<Data, Variables>> addQuery<Data, Variables>(
    QueryRef<Data, Variables> ref,
  ) {
    final queryId = ref.operationId;
    trackedQueries[queryId] = WeakReference(ref);

    final streamController =
        StreamController<QueryResult<Data, Variables>>.broadcast(
      onCancel: () {
        // Do NOT remove from trackedQueries here.
        // Let WeakReference handle cleanup when the ref is GCed.
        ref._onAllSubscribersCancelled();
      },
    );

    return streamController;
  }
  1. Update QueryRef.subscribe() to store WeakReference:
  Stream<QueryResult<Data, Variables>> subscribe() {
    _streamController ??= _queryManager.addQuery(this);
    // ...
    _queryManager.trackedQueries[operationId] = WeakReference(this);
    // ...

@dconeybe dconeybe Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broadcast StreamController is not closed.

In _onAllSubscribersCancelled, _streamController is set to null but the controller itself is not closed. Although it may eventually be garbage collected once all references are dropped, it is standard best practice to explicitly close StreamControllers to release resources immediately.

Please update QueryManager.addQuery (not in this diff) to close the controller in onCancel:

    final streamController =
        StreamController<QueryResult<Data, Variables>>.broadcast(
      onCancel: () {
        trackedQueries.remove(queryId); // Or keep it if using WeakReference
        ref._onAllSubscribersCancelled();
        streamController.close(); // Close the controller
      },
    );

}

Stream<QueryResult<Data, Variables>> subscribe() {
Expand Down
4 changes: 3 additions & 1 deletion packages/firebase_data_connect/firebase_data_connect/lib/src/firebase_data_connect.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ class FirebaseDataConnect extends FirebasePlugin {
if (ref != null) {
return ref;
} else {
return QueryRef<Data, Variables>(
final newRef = QueryRef<Data, Variables>(
this,
operationName,
transport!,
Expand All @@ -153,6 +153,8 @@ class FirebaseDataConnect extends FirebasePlugin {
varsSerializer,
vars,
);
_queryManager.trackedQueries[queryId] = newRef;
return newRef;
Comment on lines +156 to +157

@dconeybe dconeybe Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memory leak due to untracked QueryRef lifecycle.

Adding newRef to trackedQueries here means it will stay there forever if subscribe() is never called, as there is no mechanism to remove it other than when all subscribers cancel (which never happens if they never subscribe).

To fix this, we should use WeakReference to track queries.

Note: You will also need to update the lookup at lines 142-143 to retrieve the target from the WeakReference:

    final weakRef = _queryManager.trackedQueries[queryId];
    QueryRef<Data, Variables>? ref =
        (weakRef as WeakReference<QueryRef<Data, Variables>>?)?.target;
Suggested change
_queryManager.trackedQueries[queryId] = newRef;
return newRef;
_queryManager.trackedQueries[queryId] = WeakReference(newRef);
return newRef;

}
}

Expand Down
33 changes: 33 additions & 0 deletions ...ages/firebase_data_connect/firebase_data_connect/test/src/firebase_data_connect_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,5 +254,38 @@ void main() {
expect(routingTransport.websocket.auth, equals(mockAuth));
expect(routingTransport.websocket.appCheck, equals(mockAppCheck));
});

test('query returns identical QueryRef instance for identical queries', () {
final dynamicApp = DynamicMockFirebaseApp(
name: 'queryRefAppName',
options: const FirebaseOptions(
apiKey: 'fake_api_key',
appId: 'fake_app_id',
messagingSenderId: 'fake_messaging_sender_id',
projectId: 'fake_project_id',
),
);

final instance = FirebaseDataConnect(
app: dynamicApp,
connectorConfig: mockConnectorConfig,
);

final ref1 = instance.query(
'listMovies',
(json) => json,
emptySerializer,
null,
);

final ref2 = instance.query(
'listMovies',
(json) => json,
emptySerializer,
null,
);

expect(identical(ref1, ref2), isTrue);
});

@dconeybe dconeybe Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a reproduction test case that verifies the QueryRef identity is preserved even after all subscriptions are cancelled (as long as the application still holds a reference to the QueryRef).

Without the WeakReference fix, this test fails because unsubscribe removes the query from trackedQueries, causing subsequent query() calls to return a new instance.

You can append this test to the query group:

Suggested change
});
});
test('query returns identical QueryRef instance even after unsubscribe if still referenced', () async {
final dynamicApp = DynamicMockFirebaseApp(
name: 'queryRefAppName',
options: const FirebaseOptions(
apiKey: 'fake_api_key',
appId: 'fake_app_id',
messagingSenderId: 'fake_messaging_sender_id',
projectId: 'fake_project_id',
),
);
final instance = FirebaseDataConnect(
app: dynamicApp,
connectorConfig: mockConnectorConfig,
);
final ref1 = instance.query(
'listMovies',
(json) => json,
emptySerializer,
null,
);
final subscription = ref1.subscribe().listen((_) {});
await subscription.cancel();
final ref2 = instance.query(
'listMovies',
(json) => json,
emptySerializer,
null,
);
expect(identical(ref1, ref2), isTrue);
});

});
}
Loading