-
Notifications
You must be signed in to change notification settings - Fork 241
EMT-2370 setThirdPartyAPIsTimeout Implementation #1517
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… will be fetched in parallel now for installs and opens.
/matter review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation looks solid with proper timeout validation and comprehensive test coverage. Found one potential race condition in concurrent API loading.
if (!self.preferenceHelper.appleAttributionTokenChecked) { | ||
self.appleAttributionToken = [BNCSystemObserver appleAttributionToken]; | ||
if (self.appleAttributionToken) { | ||
self.preferenceHelper.appleAttributionTokenChecked = YES; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Bug Fix
Issue: Race condition when setting self.appleAttributionToken
from concurrent dispatch queue without synchronization
Fix: Add @synchronized block around property assignment to prevent data races
Impact: Prevents potential crashes or data corruption when multiple threads access this property simultaneously
if (!self.preferenceHelper.appleAttributionTokenChecked) { | |
self.appleAttributionToken = [BNCSystemObserver appleAttributionToken]; | |
if (self.appleAttributionToken) { | |
self.preferenceHelper.appleAttributionTokenChecked = YES; | |
} | |
} | |
if (!self.preferenceHelper.appleAttributionTokenChecked) { | |
NSString *token = [BNCSystemObserver appleAttributionToken]; | |
@synchronized(self) { | |
self.appleAttributionToken = token; | |
} | |
if (self.appleAttributionToken) { | |
self.preferenceHelper.appleAttributionTokenChecked = YES; | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Resolved: Apple attribution token assignment moved to synchronized block in dispatch queue
if ([[self.preferenceHelper attributionLevel] isEqualToString:BranchAttributionLevelFull]) { | ||
NSString *odmInfo = [BNCODMInfoCollector instance].odmInfo ; | ||
if (odmInfo) { | ||
[self safeSetValue:odmInfo forKey:BRANCH_REQUEST_KEY_ODM_INFO onDict:json]; | ||
if (self.odmInfo) { | ||
[self safeSetValue:self.odmInfo forKey:BRANCH_REQUEST_KEY_ODM_INFO onDict:json]; | ||
NSNumber* odmInitDateInNumberFormat = BNCWireFormatFromDate(self.preferenceHelper.odmInfoInitDate); | ||
[self safeSetValue:odmInitDateInNumberFormat forKey:BRANCH_REQUEST_KEY_ODM_FIRST_OPEN_TIMESTAMP onDict:json]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Bug Fix
Issue: Accessing self.odmInfo
without synchronization after concurrent assignment
Fix: Add @synchronized block around property access to ensure thread safety
Impact: Prevents reading potentially corrupted or partially written data
if ([[self.preferenceHelper attributionLevel] isEqualToString:BranchAttributionLevelFull]) { | |
NSString *odmInfo = [BNCODMInfoCollector instance].odmInfo ; | |
if (odmInfo) { | |
[self safeSetValue:odmInfo forKey:BRANCH_REQUEST_KEY_ODM_INFO onDict:json]; | |
if (self.odmInfo) { | |
[self safeSetValue:self.odmInfo forKey:BRANCH_REQUEST_KEY_ODM_INFO onDict:json]; | |
NSNumber* odmInitDateInNumberFormat = BNCWireFormatFromDate(self.preferenceHelper.odmInfoInitDate); | |
[self safeSetValue:odmInitDateInNumberFormat forKey:BRANCH_REQUEST_KEY_ODM_FIRST_OPEN_TIMESTAMP onDict:json]; | |
} | |
if ([[self.preferenceHelper attributionLevel] isEqualToString:BranchAttributionLevelFull]) { | |
NSString *odmInfo; | |
@synchronized(self) { | |
odmInfo = self.odmInfo; | |
} | |
if (odmInfo) { | |
[self safeSetValue:odmInfo forKey:BRANCH_REQUEST_KEY_ODM_INFO onDict:json]; | |
NSNumber* odmInitDateInNumberFormat = BNCWireFormatFromDate(self.preferenceHelper.odmInfoInitDate); | |
[self safeSetValue:odmInitDateInNumberFormat forKey:BRANCH_REQUEST_KEY_ODM_FIRST_OPEN_TIMESTAMP onDict:json]; | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Resolved: ODM info access is now properly synchronized in addODMInfoToJSON method
/matter review-full |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation looks solid with proper timeout validation and thread safety. Found one potential race condition in concurrent dispatch queue usage.
/matter summary |
/matter review |
Simple string formatting change with proper semicolon termination. Code change is minimal and safe. |
…InstallWithURLString and dataForOpenWithURLString.
/matter review-full |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comprehensive third-party API timeout implementation with proper validation and thread safety. Found several critical bugs in test code and potential race conditions.
Skipped files
Branch-TestBed/Branch-TestBed.xcodeproj/project.pbxproj
: Skipped file patternBranch-TestBed/Reflection_ODM_Tests/Framework/AppAdsOnDeviceConversion.xcframework/Info.plist
: Skipped file patternBranch-TestBed/Reflection_ODM_Tests/Framework/AppAdsOnDeviceConversion.xcframework/ios-arm64/AppAdsOnDeviceConversion.framework/AppAdsOnDeviceConversion
: File hunk diff too largeBranch-TestBed/Reflection_ODM_Tests/Framework/AppAdsOnDeviceConversion.xcframework/ios-arm64/AppAdsOnDeviceConversion.framework/Info.plist
: Skipped file patternBranch-TestBed/Reflection_ODM_Tests/Framework/AppAdsOnDeviceConversion.xcframework/ios-arm64_x86_64-simulator/AppAdsOnDeviceConversion.framework/AppAdsOnDeviceConversion
: File hunk diff too largeBranch-TestBed/Reflection_ODM_Tests/Framework/AppAdsOnDeviceConversion.xcframework/ios-arm64_x86_64-simulator/AppAdsOnDeviceConversion.framework/Info.plist
: Skipped file pattern
/matter review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation looks solid with proper timeout validation and concurrent API loading. Found one test assertion issue and a potential race condition in property access.
Skipped files
Branch-TestBed/Branch-TestBed.xcodeproj/project.pbxproj
: Skipped file patternBranch-TestBed/Reflection_ODM_Tests/Framework/AppAdsOnDeviceConversion.xcframework/Info.plist
: Skipped file patternBranch-TestBed/Reflection_ODM_Tests/Framework/AppAdsOnDeviceConversion.xcframework/ios-arm64/AppAdsOnDeviceConversion.framework/AppAdsOnDeviceConversion
: File hunk diff too largeBranch-TestBed/Reflection_ODM_Tests/Framework/AppAdsOnDeviceConversion.xcframework/ios-arm64/AppAdsOnDeviceConversion.framework/Info.plist
: Skipped file patternBranch-TestBed/Reflection_ODM_Tests/Framework/AppAdsOnDeviceConversion.xcframework/ios-arm64_x86_64-simulator/AppAdsOnDeviceConversion.framework/AppAdsOnDeviceConversion
: File hunk diff too largeBranch-TestBed/Reflection_ODM_Tests/Framework/AppAdsOnDeviceConversion.xcframework/ios-arm64_x86_64-simulator/AppAdsOnDeviceConversion.framework/Info.plist
: Skipped file pattern
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
1 similar comment
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
Reference
EMT-2370 - https://branch.atlassian.net/browse/EMT-2370
Type Of Change
cc @BranchMetrics/saas-sdk-devs for visibility.