-
Notifications
You must be signed in to change notification settings - Fork 135
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
RUM-3175 fix: Fix race condition during consent change (pending β granted) #2063
Conversation
Datadog ReportBranch report: β
0 Failed, 3473 Passed, 0 Skipped, 3m 35.42s Total Time π» Code Coverage Decreases vs Default Branch (2) |
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.
The flush
is a sync method:
/// Flushes asynchronous operations related to events write, context and message bus propagation in this instance of the SDK
/// with **blocking the caller thread** till their completion.
We should not block the user thread here, the flush method is mostly used for testing.
Instead, it might be sufficient to just publish the new consent before doing the migration. Or to call the migration from the context queue, also after publishing the new consent.
I would also be great to have a test case covering this, to prevent regression.
7a2ce97
to
988a354
Compare
071fdd3
to
c765a57
Compare
this prevents loss of events recorded on the current thread (right before the consent was changed).
817a0e1
to
1cdb2a9
Compare
@maxep Fair call β . I continued this work fixing it in proposed way βοΈ. It's not necessary to publish the consent before migration task as migration depends on the new consent value explicitly. |
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.
Nice, on target π― π
/merge |
π MergeQueue: pull request added to the queue The median merge time in Use |
π¨ MergeQueue: This merge request is in error mergequeue build completed successfully, but the github api returned an error while merging the pr.
DetailsError: PUT https://api.github.com/repos/DataDog/dd-sdk-ios/pulls/2063/merge: 405 Required status check "dd-gitlab/Sync GH Checks" is expected. [] FullStacktrace: If you need support, contact us on Slack #devflow with those details! |
/merge |
π MergeQueue: waiting for PR to be ready This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals. Use |
π MergeQueue: pull request added to the queue The median merge time in Use |
What and why?
π¦π This PR fixes a race condition that occurs during the change of tracking consent from
.pending
to.granted
. This issue could result in the loss of events recorded on the current thread right before the consent change.How?
The fix involves synchronizing data migration on the context queue to prevent conflicts with ongoing "event write" operations. This change guarantees that all latest events are accurately migrated based on the updated consent state.
Review checklist