Removed Auto Initialization#1571
Conversation
Added API - `- (void)setConsumerProtectionAttributionLevel:(BranchAttributionLevel)level resetSession:(BOOL)resetSession` to provide an option to disable resetting session. Remove silent SDK initialization - initSafetyCheck removed.Now APIs will return or log BNCInitError error. Added @synchronized(self) around all 5 init status checks to ensure it waits for any in-progress initSession code (which also uses @synchronized(self)) to finish and update the status before evaluating.
There was a problem hiding this comment.
🧪 PR Review is completed: Critical review focusing on thread safety. The introduction of @synchronized(self) combined with synchronous main-thread dispatching creates significant deadlock risks in several methods. These should be converted to asynchronous dispatches to match the existing async nature of these methods and prevent deadlocks.
|
@gdeluna-branch Should I call all callbacks reporting error on main thread ? Currenlty I have copied the behavior from the success path - how(sync/asynchronous) and where (main/same thread) callback is called when API succeeds. |
| if ([[BNCCallbackMap shared] containsRequest:request]) { | ||
| dispatch_async(dispatch_get_main_queue(), ^{ | ||
| [[BNCCallbackMap shared] callCompletionForRequest:request withSuccessStatus:NO error:error]; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟡 Concurrency / Thread Safety
Issue: Checking containsRequest: on the current thread (which might be a background thread) creates a potential race condition. The request could be removed from the map between the check and the asynchronous execution. Additionally, if BNCCallbackMap is not thread-safe, accessing it from a background thread is risky.
Fix: Move the containsRequest: check inside the dispatch_async block. This ensures the check and execution happen atomically on the main thread and aligns with the pattern used in other hunks.
Impact: Improves thread safety and prevents potential race conditions.
| if ([[BNCCallbackMap shared] containsRequest:request]) { | |
| dispatch_async(dispatch_get_main_queue(), ^{ | |
| [[BNCCallbackMap shared] callCompletionForRequest:request withSuccessStatus:NO error:error]; | |
| }); | |
| } | |
| dispatch_async(dispatch_get_main_queue(), ^{ | |
| if ([[BNCCallbackMap shared] containsRequest:request]) { | |
| [[BNCCallbackMap shared] callCompletionForRequest:request withSuccessStatus:NO error:error]; | |
| } | |
| }); |
Yes since the callbacks return UI data or error for UI we should consistently return on main for both paths. |
- (void)setConsumerProtectionAttributionLevel:(BranchAttributionLevel)level resetSession:(BOOL)resetSessionto provide an option to disable resetting session.Reference
SDK-XXXX -- <TITLE>.
Summary
Motivation
Type Of Change
Testing Instructions
cc @BranchMetrics/saas-sdk-devs for visibility.