Set pending shared state before updating Identity Map #81
Conversation
This file contains 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
Codecov Report
@@ Coverage Diff @@
## dev-v2.0.0 #81 +/- ##
==============================================
+ Coverage 93.32% 93.39% +0.07%
==============================================
Files 12 12
Lines 674 681 +7
Branches 103 103
==============================================
+ Hits 629 636 +7
Misses 16 16
Partials 29 29
Flags with carried forward coverage won't be shown. Click here to find out more.
|
emdobrin
reviewed
Jan 11, 2023
code/edgeidentity/src/main/java/com/adobe/marketing/mobile/edge/identity/IdentityExtension.java
Show resolved
Hide resolved
| @@ -268,8 +269,10 @@ void handleUpdateIdentities(@NonNull final Event event) { | |||
| return; | |||
| } | |||
|
|
|||
| // Add pending shared state to avoid race condition between updating and reading identity map | |||
| final SharedStateResolver resolver = getApi().createPendingXDMSharedState(event); | |||
There was a problem hiding this comment.
would this be fast enough or can we move this before the checks and conversions above as we do in iOS?
emdobrin
reviewed
Jan 12, 2023
| @@ -265,11 +270,12 @@ void handleUpdateIdentities(@NonNull final Event event) { | |||
| LOG_SOURCE, | |||
| "Failed to update identifiers as no identifiers were found in the event data." | |||
| ); | |||
| resolver.resolve(state.getIdentityProperties().toXDMData(false)); | |||
There was a problem hiding this comment.
unrelated to your change, but looks like toXDMData(allowEmpty) is called with false in all cases, can we remove that param? 😁
There was a problem hiding this comment.
The iOS code sets the value to true when handling identity and get identifiers requests, so I'll leave this as is for now and update if needed when I work on syncing the iOS and Android implementations.
emdobrin
approved these changes
Jan 12, 2023
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.
Set a pending shared state before update and remove Identity Map operations.
Description
A race condition can occur when updating the IdentityMap followed by an Edge.sendEvent() call. The issue is the Identity extension doesn't finish processing the update to the IdentityMap before the Edge extension retrieves the Identity shared state. This causes the Edge Network request to use the IdentityMap before the update.
The fix is to simply create a pending shared state before any updates to the Identity Map in IdentityProperties. After the Identity Map is edited, the pending shared state is resolved with the new Identity Map XDM.
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: