fix(functions): thread-safe Android streaming listener lifecycle#9093
fix(functions): thread-safe Android streaming listener lifecycle#9093mikehardy wants to merge 2 commits into
Conversation
|
Bugbot is not enabled for this team, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the thread-safety of Android functions streaming by synchronizing access to listener management and adjusting the cleanup logic to prevent lost events. Additionally, it enforces stricter quality standards by integrating a mandatory coverage evidence gate for native bridge and library changes, supported by updated documentation and new e2e test cases. Highlights
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. Footnotes
|
![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 new coverage_evidence gate to the validation and documentation workflow, detailing evidence requirements and anti-patterns. In the Android codebase, thread synchronization was added around functionsStreamingListeners to prevent race conditions. However, the review feedback correctly identifies that removing the listener cleanup from the success, error, and exception paths in NativeRNFBTurboFunctions introduces memory leaks, as completed or failed listeners are never removed from the static map. The reviewer provides actionable suggestions to safely remove these listeners from the map without triggering a cancellation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Synchronize access to the streaming listeners SparseArray and defer removal until explicit cancel or invalidate so final stream events are not dropped under concurrent unsubscribe. Add e2e coverage for native streaming errors and early stream cancel.
Add coverage_evidence_gate, mandatory Jacoco/lcov evidence package, NYC anti-pattern, and subagent return fields so native/lib bridge changes cannot close review without per-file coverage proof.
mikehardy
force-pushed
the
functions-threadsafety
branch
from
c5187ce to
b04fb15
Compare
|
Bugbot is not enabled for this team, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9093 +/- ##
============================================
+ Coverage 58.61% 58.64% +0.03%
- Complexity 1616 1618 +2
============================================
Files 503 503
Lines 39178 39190 +12
Branches 5807 5808 +1
============================================
+ Hits 22962 22980 +18
+ Misses 14811 14744 -67
- Partials 1405 1466 +61
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| Object listener = functionsStreamingListeners.get(listenerId); | ||
| if (listener != null) { | ||
| if (listener instanceof Subscription) { | ||
| ((Subscription) listener).cancel(); |
There was a problem hiding this comment.
after putting this through review, it's safer to cancel() outside of lock, if cancel stalls or hangs then this could deadlock which makes sense I think
There was a problem hiding this comment.
The API contract for cancel on a Reactive Streams instance indicates it must be timely and non-obstructive so it would be a contract violation for to hang or stall. In my execution path analysis I couldn't suss out a deadlock path but those are seemingly always surprising, and it never hurts to completely isolate work that strictly requires the lock from any other work. I think contention is more likely and sufficient as an argument against holding the lock either way.
So I definitely take the point. I'm hanging up on this one myself as I re-think it because I feel like the locking pattern growing here (and in response to this comment) is starting to get complicated and "complicated locking" isn't something to play with. I also noticed the same pattern in Firestore on android, and in a couple places on iOS - all exhibiting the same latent / "we have been lucky so far" unlocked listener maps / concurrent teardown windows possibilities
Let me finish my analysis there and the final result will I think be an architecture decision with a generic policy on listener registry design + code patterns, then a refactor of listener registries on both platform in all modules
There was a problem hiding this comment.
yeah, I'm also in two minds. I feel it would never hang, and would be fixed upstream in that event probably. but then I think - is it worth the risk? 🤔
| **Verdict line:** `100% on reachable touched lines` **or** `NOT 100%` with numbered gaps and disposition (fixed / intractable with evidence / user-accepted deferral). | ||
|
|
||
| Reviewers treat missing or stale coverage evidence as a **blocking** finding — same bar as missing e2e counts ([change authoring § validation evidence](change-authoring-workflow.md#validation-evidence-blocking)). | ||
| Record in `.agents/reports/<item>/coverage-evidence.md` (preferred) or work-queue notes. Orchestrators **must not** close `coverage_evidence_gate` or `review_gate` without this file when lib/native bridge is touched. |
There was a problem hiding this comment.
"without this file" - could this be clarified? Review says change to "this evidence package", but it could also be "without changing this file"?
There was a problem hiding this comment.
just the sort of ambiguity that can lead to drift - good catch - I'm changing to "without recording the evidence in one of those locations"
2 participants
Description
Synchronizes access to the Android functions streaming listeners
SparseArrayand defers listener removal until explicit cancel or moduleinvalidate(), so a concurrent unsubscribe does not drop the final stream event.Also adds OKF coverage-evidence gate requirements (
coverage_evidence_gate, Jacoco/lcov evidence package, NYC anti-pattern) so native bridge changes must prove per-file coverage before review closes.Related issues
N/A — proactive thread-safety hardening for Android streaming callables.
Release Summary
Fixes a race where Android functions streaming could lose the final result when unsubscribe raced with stream completion.
Checklist
AndroidiOSOther(macOS, web)e2etests added or updated inpackages/**/e2ejesttests added or updated inpackages/**/__tests__Test Plan
yarn tests:jest --watchman=false packages/functions/__tests__/— 11/11 passapp+functions) — 76 passing, 1 pending —/tmp/rnfb-e2e-functions-threadsafety-android-r4.logyarn tests:android:post-e2e-coverage— Jacoco report forNativeRNFBTurboFunctions.java; core sync/defer-removal paths covered; four defensive/teardown branches documented as user-accepted deferrals in.agents/reports/functions-threadsafety/coverage-evidence.md