Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Bug] The user executor needs to uncache before other executors #1465

Merged
merged 3 commits into from
Aug 2, 2024
Merged

[Bug] The user executor needs to uncache before other executors #1465

merged 3 commits into from
Aug 2, 2024
+169 −147

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Jul 26, 2024 •
edited
Loading

Description

One Line Summary

The User Executor is no longer a static class, and will finish uncaching before other executors uncache.

Details

User executor is no longer a static class

  • Primary Motivation: After a dispatch queue was added to the user executor, it's uncaching can happen on a background thread. However, when the User Manager starts, the user executor needs to finish uncaching first before other executors begin uncaching. This is because their uncached requests rely on the identity models organized by the user executor. By happening on a background thread, this order is not guaranteed. By making the User Executor an instance instead of a static class, it can uncache in its initializer sans dispatch queue without concurrency dangers.
  • Additional motivations include Identity Verification work that allows the user executor to conform to the same listener protocol as other executors.
  • All other executors are instances owned by the User Manager.
  • Make the User Executor also an instance owned by the User Manager, as it is the only class that calls it.
  • Access to user executor is force unwrapped ‼️ because everything is guarded behind the User Manager's start() call. This will surface any issues or wrong usages early.
  • The reason for the force unwrapping is because the User Manager and the executors have circular dependency currently, unfortunately

Motivation

Bug Fix

Scope

Testing

Unit testing

  • no new tests added but minor updates made

Manual testing

Device: iPhone 13 on iOS 17.5.1
Reproduce original wrong behavior:

  1. Start with an anonymous user and turn off internet connection
  2. Login to userA and add tag for userA
  3. Login to userB and add tag for userB
  4. Note this will enqueue an IdentifyUser userA, CreateUser userB in the user executor
  5. Note this will enqueue two UpdateUser in the property executor
  6. Swipe away the app, turn on internet, and re-open app
  7. The user executor does not finish uncaching before the property executor uncaches
  8. Therefore, UpdateUser userA is dropped as the property executor cannot connect the Identity model reference (needed to know when the OSID is hydrated)

After changes in this PR:

  • Both tag updates are sent to their respective users

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is Reviewable

@nan-li
User executor is no longer a static class
6eda3bf 
* Primary Motivation: After a dispatch queue was added to the user executor, it's uncaching can happen on a background thread. However, when the User Manager starts, the user executor needs to finish uncaching first before other executors begin uncaching. This is because their uncached requests rely on the identity models organized by the user executor. By happening on a background thread, this order is not guaranteed. By making the User Executor an instance instead of a static class, it can uncache in its initializer without concurrency dangers.
* Additional motivations include Identity Verification work that allows the user executor to conform to the same listener protocol as other executors.
* All other executors are instances owned by the User Manager.
* Make the User Executor also an instance owned by the User Manager, as it is the only class that calls it.
* Access to user executor is force unwrapped because everything is guarded behind the User Manager's `start()` call. This will surface any issues or wrong usages early.
@nan-li
nit: Fix a string description to simplify tests
eede113 
* Fix the string description of OSRequestCreateUser to be uniform
@nan-li nan-li requested a review from emawby July 26, 2024 01:40
@nan-li
no logic change - organize executor startup code
eb58e4f 
* No logic changes here, just break up executor code into digestible functions
@emawby emawby self-assigned this Jul 29, 2024
@nan-li nan-li requested a review from jkasten2 July 29, 2024 17:38
emawby
emawby suggested changes Jul 29, 2024
View reviewed changes
Copy link
Contributor

@emawby emawby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like some of the switch user integration tests failed in the CI. When I ran it locally they didn't fail but other tests are failing every time

@nan-li
Copy link
Contributor Author

nan-li commented Jul 29, 2024

Looks like some of the switch user integration tests failed in the CI. When I ran it locally they didn't fail but other tests are failing every time

A live activity executor test, right?

@emawby
Copy link
Contributor

emawby commented Jul 30, 2024

Looks like some of the switch user integration tests failed in the CI. When I ran it locally they didn't fail but other tests are failing every time

A live activity executor test, right?

No it is a notification enters foreground test. It seems like they are probably all unrelated to this PR and are just flaky

@emawby emawby self-requested a review July 30, 2024 15:55
emawby
emawby approved these changes Jul 30, 2024
View reviewed changes
Copy link
Contributor

@emawby emawby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jkasten2)

Base automatically changed from feat/no_more_external_id_based_requests to main August 2, 2024 00:21
@nan-li nan-li merged commit 836c73a into main Aug 2, 2024
2 of 3 checks passed
@nan-li nan-li deleted the fix/user_executor_starts_first branch August 2, 2024 00:22
This was referenced Aug 9, 2024
Merged
Merged
@emawby emawby mentioned this pull request Aug 14, 2024
Merged
This was referenced Aug 15, 2024
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emawby emawby emawby approved these changes

@jkasten2 jkasten2 Awaiting requested review from jkasten2

Assignees

@emawby emawby

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nan-li @emawby