[FSSDK-12346] fix: defer NWPathMonitor creation to avoid crash during multi-SDK startup#650
Merged
Merged
Conversation
…rtup NWPathMonitor was eagerly created in NetworkReachability.init, which runs during DefaultEventDispatcher stored-property initialization. In Flutter apps with multiple SDKs (Firebase, Datadog, MParticle) on iOS 16, this caused a null function pointer crash (EXC_BAD_ACCESS at 0x0) due to dyld contention over the weak-linked Network framework during concurrent framework loading. - Defer NWPathMonitor creation from init to first shouldBlockNetworkAccess() call - Remove force casts (as! NWPathMonitor) in favor of local variable binding - Prevent monitor start after stop() to maintain test isolation - Add tests verifying monitor is not created during init Fixes: FSSDK-12346 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Defers NWPathMonitor initialization in NetworkReachability until first use to avoid early Network framework symbol resolution during multi-SDK startup, and adds tests to verify the new lifecycle behavior.
Changes:
- Moved
NWPathMonitorcreation out ofinitinto a newstartMonitorIfNeeded()method, invoked on firstshouldBlockNetworkAccess()call. - Added
monitorStartedgating and updatedstop()behavior to prevent monitor creation after teardown. - Added/updated unit tests to assert monitor creation timing and
stop()behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
Sources/Utils/NetworkReachability.swift |
Defers monitor creation to first use and adds lifecycle gating around starting/stopping the monitor. |
Tests/OptimizelyTests-Common/NetworkReachabilityTests.swift |
Adds tests ensuring the monitor is not created during init and not created after stop(), and updates monitoring test to explicitly start the monitor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
coverage: 93.889% (+0.03%) from 93.859% — fix/lazy-nwpathmonitor-init into master |
numContiguousFails was read in shouldBlockNetworkAccess() and written in updateNumContiguousFails() from different threads without synchronization. Both now use queue.sync for consistent thread-safe access. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Hammers updateNumContiguousFails and shouldBlockNetworkAccess from 1000 concurrent threads to verify no data race or crash. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Make all mutable state private (monitor, connected, numContiguousFails, maxContiguousFails, monitorStarted) — accessed only within queue - Provide thread-safe computed accessors (isConnected, numContiguousFails, maxContiguousFails, isMonitorActive) for external use - Read numContiguousFails and connected in a single queue.sync snapshot in shouldBlockNetworkAccess() to eliminate TOCTOU race - Move monitor creation and cancellation fully inside queue.sync, with only NWPathMonitor.start(queue:) outside to avoid deadlock - Set monitor = nil in stop() for clean resource release - Update tests to use isMonitorActive instead of accessing monitor directly Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
raju-opti
reviewed
Jul 8, 2026
raju-opti
approved these changes
Jul 8, 2026
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
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.
Summary
NWPathMonitorcreation fromNetworkReachability.initto firstshouldBlockNetworkAccess()callas! NWPathMonitor) in favor of local variable bindingmonitorStartedflag instop()to prevent late monitor creation after teardownstop()In Flutter apps with multiple SDKs (Firebase, Datadog, MParticle) on iOS 16,
NWPathMonitorwas eagerly created duringDefaultEventDispatcherstored-property initialization. This caused a null function pointer crash (EXC_BAD_ACCESSat0x0) due to dyld contention over the weak-linked Network framework during concurrent framework loading. The crash did not reproduce on iOS 17+ (improved dyld concurrency) or in sample apps (no competing SDKs).By deferring
NWPathMonitorcreation to first use during event dispatch, the Network framework symbols are resolved well after app startup contention has settled.Test plan
testMonitorNotCreatedDuringInit— confirmsmonitoris nil after init, non-nil aftershouldBlockNetworkAccess()testMonitorNotCreatedAfterStop— confirms monitor stays nil ifstop()was called before first usetestReachabilityMonitoringto explicitly trigger monitor startNetworkReachabilityTestsandEventDispatcherRetryTestspassIssues