fix(firestore): remote target ID mapping to avoid possible race condition on rapid subscribe and unsubscribe#8108
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
📝 PRs merging into main branchOur main branch should always be in a releasable state. If you are working on a larger change, or if you don't want this change to see the light of the day just yet, consider using a feature branch first, and only merge into the main branch when the code complete and ready to be released. |
Generated by 🚫 Danger |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mapping layer between SDK target IDs and session-specific remote target IDs to facilitate target reuse and improve watch stream management. Key changes include the addition of the RemoteTargetId class, specialized ID generators, and translation logic in RemoteStore and WatchChangeAggregator. Feedback highlights performance concerns in RemoteStore, specifically identifying O(N^2) complexity in raiseWatchSnapshot when processing target changes and mismatches, as well as inefficient repeated canonical ID calculations for pipelines within the listen method's loop.
| private String getPipelineCanonicalIdExceptSort( | ||
| com.google.firebase.firestore.RealtimePipeline p) { | ||
| String canonicalId = p.toString(); | ||
| String[] stages = canonicalId.split("\\|"); | ||
| List<String> stripped = new ArrayList<>(); | ||
| for (String stage : stages) { | ||
| if (!stage.startsWith("sort")) { | ||
| stripped.add(stage); | ||
| } | ||
| } | ||
| return String.join("|", stripped); | ||
| } |
There was a problem hiding this comment.
Where did this come from?
| private int generateRemoteTargetId(int sdkTargetId) { | ||
| if (sdkTargetId % 2 != 0) { | ||
| return syncEngineTargetIdGenerator.nextId(); | ||
| } else { | ||
| return targetCacheTargetIdGenerator.nextId(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This should return the type RemoteTargetId
| if (remoteTargetId != null) { | ||
| targetIdMapRemoteToSdk.remove(remoteTargetId); | ||
| } | ||
| remoteTargetId = RemoteTargetId.from(generateRemoteTargetId(sdkTargetId)); | ||
| targetIdMapSdkToRemote.put(sdkTargetId, remoteTargetId); | ||
| targetIdMapRemoteToSdk.put(remoteTargetId, sdkTargetId); |
There was a problem hiding this comment.
THis appears to be ported from allocateRemoteTargetId. Keep that helper method
|
|
||
| if (remoteTargetId != null && listenTargets.containsKey(remoteTargetId)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
move this short circuit execution to the top
| return String.join("|", stripped); | ||
| } | ||
|
|
||
| public void listen(TargetData targetData) { |
There was a problem hiding this comment.
port the debug logging in this method from remoteStoreListen
| if (remoteTargetId == null) { | ||
| TargetOrPipeline top = targetData.getTarget(); | ||
| String topCanonicalId = | ||
| top.isPipeline() | ||
| ? getPipelineCanonicalIdExceptSort( | ||
| top.pipeline$com_google_firebase_firebase_firestore()) | ||
| : null; | ||
|
|
||
| for (Map.Entry<RemoteTargetId, TargetData> entry : listenTargets.entrySet()) { | ||
| TargetOrPipeline activeTop = entry.getValue().getTarget(); | ||
| if (top.isTarget() && activeTop.isTarget()) { | ||
| if (top.target().equals(activeTop.target())) { | ||
| remoteTargetId = entry.getKey(); | ||
| targetIdMapSdkToRemote.put(sdkTargetId, remoteTargetId); | ||
| break; | ||
| } | ||
| } else if (top.isPipeline() && activeTop.isPipeline()) { | ||
| if (topCanonicalId.equals( | ||
| getPipelineCanonicalIdExceptSort( | ||
| activeTop.pipeline$com_google_firebase_firebase_firestore()))) { | ||
| remoteTargetId = entry.getKey(); | ||
| targetIdMapSdkToRemote.put(sdkTargetId, remoteTargetId); | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Document this code. What does it do, why do we need it?
| TargetData remoteTargetData = | ||
| new TargetData( | ||
| targetData.getTarget(), | ||
| remoteTargetId.value(), |
There was a problem hiding this comment.
Should TargetData be generic, like in the web SDK, so we don't have to unbox the remoteTargetId and lose our type safety
| hardAssert( | ||
| targetData != null, "stopListening called on target no currently watched: %d", targetId); | ||
| remoteTargetId != null, | ||
| "stopListening called on target not currently watched: %d", | ||
| sdkTargetId); |
There was a problem hiding this comment.
this is duplicated. Add the debug logging from the web SDK
| } | ||
| } | ||
|
|
||
| private void sendWatchRequest(TargetData targetData) { |
There was a problem hiding this comment.
First param should be type TargetData<RemoteTargetId>. Then update the implementation
| if (!targetData.getResumeToken().isEmpty() | ||
| || targetData.getSnapshotVersion().compareTo(SnapshotVersion.NONE) > 0) { | ||
| int expectedCount = this.getRemoteKeysForTarget(targetData.getTargetId()).size(); | ||
| int expectedCount = this.getRemoteKeysForTarget(remoteTargetId).size(); |
There was a problem hiding this comment.
getRemoteKeysForTarget takes an sdkTargetId, so you'll have to look that up first
2 participants
No description provided.