fix: Make setSessionId synchronous to prevent race with MPRoktLayout#735
fix: Make setSessionId synchronous to prevent race with MPRoktLayout#735nickolas-dimitrakas wants to merge 1 commit into
Conversation
setSessionId was using two async main queue hops (via forwardSDKCall) before reaching the Rokt SDK, causing a race condition when followed immediately by MPRoktLayout which reads the session synchronously. Use the same direct kit access pattern as getSessionId, falling back to forwardSDKCall only when kits haven't initialized yet. Made-with: Cursor
PR SummaryMedium Risk Overview Preserves existing behavior by falling back to the deferred Reviewed by Cursor Bugbot for commit c480e95. Bugbot is set up for automated code reviews on this repo. Configure here. |
📦 SDK Size Impact ReportMeasures how much the SDK adds to an app's size (with-SDK minus without-SDK).
➡️ SDK size impact change is minimal. Raw measurementsTarget branch (workstation/8.x-Maintenance): {"baseline_app_size_kb":84,"baseline_executable_size_bytes":75464,"with_sdk_app_size_kb":1948,"with_sdk_executable_size_bytes":76360,"sdk_impact_kb":1864,"sdk_executable_impact_bytes":896,"xcframework_size_kb":9736}This PR: {"baseline_app_size_kb":84,"baseline_executable_size_bytes":75464,"with_sdk_app_size_kb":1948,"with_sdk_executable_size_bytes":76360,"sdk_impact_kb":1864,"sdk_executable_impact_bytes":896,"xcframework_size_kb":9732} |
| NSArray<id<MPExtensionKitProtocol>> *activeKits = | ||
| [[MParticle sharedInstance].kitContainer_PRIVATE activeKitsRegistry]; | ||
|
|
||
| for (id<MPExtensionKitProtocol> kitRegister in activeKits) { |
There was a problem hiding this comment.
setSessionId is only used on rokt object. So iterating through other kits is not required.
There was a problem hiding this comment.
since its an array he has to iterate through until he finds the rokt kit.
| } | ||
| } | ||
|
|
||
| MPILogDebug(@"MPRokt setSessionId - kit not available, queueing for deferred execution"); |
There was a problem hiding this comment.
So you're both setting it directly and then setting it throught the normal kit container way?
There was a problem hiding this comment.
Yes, I'm still using the previous logic in the case that the kit is not configured yet
| NSArray<id<MPExtensionKitProtocol>> *activeKits = | ||
| [[MParticle sharedInstance].kitContainer_PRIVATE activeKitsRegistry]; | ||
|
|
||
| for (id<MPExtensionKitProtocol> kitRegister in activeKits) { |
There was a problem hiding this comment.
We can extract loop in one method getRockKit, If we have not specific property to directly access kit
| // Test helper class that simulates a kit with getSessionId and setSessionId methods | ||
| @interface MPRoktTestKitInstance : NSObject | ||
| @property (nonatomic, copy) NSString *sessionIdToReturn; | ||
| @property (nonatomic, copy) NSString *receivedSessionId; |
There was a problem hiding this comment.
You can just create property sessionId instead additional getter and setter.
| - (void)setSessionId:(NSString *)sessionId; | ||
| @end | ||
|
|
||
| @implementation MPRoktTestKitInstance |
There was a problem hiding this comment.
In general about the test, shouldn't we create new test in swift?
There was a problem hiding this comment.
Went for speed here, I will try to create the tests in swift if it is not too time consuming. Otherwise I can try to address this in a separate PR
4 participants
Background
A race condition in
setSessionIdwas causing web-to-native session handoff to fail on iOS. The method was delivered asynchronously via twodispatch_async(main)hops throughforwardSDKCall, whileMPRoktLayoutreads the session synchronously fromUserDefaultsduring execute. This meant the session ID was missing when both calls were made in quick succession. Android was unaffected because itssetSessionIdpath is synchronous.What Has Changed
setSessionIduse the same direct kit access pattern thatgetSessionIdalready uses, bypassingforwardSDKCalland delivering the session ID synchronously to the kitforwardSDKCallwhen kits haven't initialized yet, preserving existing queuing behaviortestSetSessionIdForwardsToKitContainertest with three new tests covering synchronous delivery,forwardSDKCallbypass, and fallback behaviorTimeline
Click to see Sequence Diagram: setSessionId and MPRoktLayout Execute
Checklist