Migrate Connectivity Monitoring to NWPathMonitor on Apple Platforms#16112
Migrate Connectivity Monitoring to NWPathMonitor on Apple Platforms#16112cherylEnkidu wants to merge 33 commits into
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 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 counter productive. 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. |
|
@ncooke3 Hi Nick, I’m still working on this PR. The replacement is less intuitive than I expected. I’ll ping you once the code is ready. :) |
| if (!snapshot.empty && !snapshot.metadata.fromCache) { | ||
| if (!snapshot.empty && !snapshot.metadata.fromCache && | ||
| !snapshot.metadata.hasPendingWrites) { | ||
| [testExpectiation fulfill]; |
There was a problem hiding this comment.
Fix flaky testWatchSurvivesNetworkDisconnect under new connectivity timing.
The test used addSnapshotListenerWithIncludeMetadataChanges:YES and a filter of !empty && !fromCache, expecting the listener to fulfill the expectation exactly once. With metadata changes enabled, the listener fires twice in the recovery sequence: once when the snapshot is raised from the server with hasPendingWrites=true (write enqueued, awaiting ack), and again when hasPendingWrites flips to false after server ack. Both pass the existing filter, causing over-fulfill.
This is a latent bug in the test, not a regression from the connectivity monitor refactor. The connectivity changes in this PR shifted the timing of network state delivery in a way that exposed it.
Tighten the filter to also require !hasPendingWrites, which is the condition the test actually intends to wait for: the write has fully round-tripped to the server.
|
|
||
| nw_path_monitor_start(monitor_); | ||
|
|
||
| #if TARGET_OS_IOS || TARGET_OS_TV || TARGET_OS_VISION |
There was a problem hiding this comment.
Unlike the previous SCNetworkReachability implementation, NWPathMonitor delivers its initial status asynchronously, creating a brief, rare window upon resuming from the background where a nullopt status might cause the foreground handler to skip forced callbacks, resulting in at worst a single missed eager connection reset before the subsequent update arrives.
In theory this window is tiny and the notification does not fire on cold launch, so the race is unlikely
to be observable. I would prefer to keep this logic simple instead of adding extra code block to handle this edge case.
| ConnectivityMonitorApple::~ConnectivityMonitorApple() { | ||
| HARD_ASSERT(this->queue()->IsCurrentQueue(), | ||
| "ConnectivityMonitorApple must be destroyed on its AsyncQueue. " | ||
| "See class comment for why."); |
There was a problem hiding this comment.
This assertion is mainly used for ensuring the test is been implemented probably. In production, both ~FirestoreClient() and FirestoreClient::TerminateInternal() would make sure ConnectivityMonitorApple is destroyed on the AsyncQueue.
|
./gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates the network connectivity monitoring implementation for Apple platforms from the legacy SCNetworkReachability API to the modern NWPathMonitor API. The changes include updating build configurations, adding a new connectivity monitor implementation, and updating the Firestore client termination logic. I have identified a few minor improvements: some redundant includes should be removed, and the indentation of the empty block in the dispatch_sync call should be corrected for consistency.
| # 12.13.0 | ||
| - [feature] Added search stage support for `languageCode`, `offset`, `limit`, and `retrievalDepth`. | ||
| - [feature] Added support for Pipeline expressions `arraySlice`, `arrayFilter`, `arrayTransform` and `arrayTransformWithIndex`. (#16001) | ||
| - [fixed] Add missing `noexcept` specifiers to move, hash, swap operations [#16117]. | ||
| - [changed] Migrates the network connectivity monitoring implementation for Apple platforms from the legacy SCNetworkReachability API to the modern NWPathMonitor API. |
There was a problem hiding this comment.
nit: 80 chars wrapping and put new entries under new "Unreleased" header
| - [changed] Migrates the network connectivity monitoring implementation for Apple platforms from the legacy SCNetworkReachability API to the modern NWPathMonitor API. | |
| # Unreleased | |
| - [changed] Migrates the network connectivity monitoring implementation for | |
| Apple platforms from the legacy SCNetworkReachability API to the modern | |
| NWPathMonitor API. | |
| # 12.13.0 | |
| - [feature] Added search stage support for `languageCode`, `offset`, `limit`, and `retrievalDepth`. | |
| - [feature] Added support for Pipeline expressions `arraySlice`, `arrayFilter`, `arrayTransform` and `arrayTransformWithIndex`. (#16001) | |
| - [fixed] Add missing `noexcept` specifiers to move, hash, swap operations [#16117]. |
Design detail: go/firestore-nwpathmonitor
Summary
This PR migrates the network connectivity monitoring implementation for Apple platforms from the legacy
SCNetworkReachabilityAPI to the modernNWPathMonitorAPI. This modernizes the codebase and resolves potential issues with older network reachability APIs, while adding robust thread-safety guards during initialization and destruction.Key Changes
Firestore Core / Remote
NWPathMonitor: ReplacedSCNetworkReachabilitywithNWPathMonitorinConnectivityMonitorApple(connectivity_monitor_apple.mm).ConnectivityMonitorApplemust be both constructed and destructed on theAsyncQueueto prevent race conditions and use-after-free bugs with the monitor's handlers.FirestoreClient::TerminateInternal()in firestore_client.cc to reset theconnectivity_monitor_last, ensuring it is destroyed on theAsyncQueueafter all callback registrants are gone.Utilities
IsCurrentQueue()toAsyncQueueto allow components to verify they are running on the correct queue (used inConnectivityMonitorAppledestruction).Tests
FSTConnectivityMonitorTests.mmto verify thatUIApplicationWillEnterForegroundNotificationcorrectly triggers callbacks when the network is available.self.db = [self firestore];line inFIRIndexingTests.mmsetup.Build & Dependencies
Networkframework toFirebaseFirestoreInternal.podspecandPackage.swiftfor supported Apple platforms.Testing Done
FSTConnectivityMonitorTests.mmto verify foreground notification behavior.