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

Fix user executor's requests from being blocked #1398

Merged
merged 5 commits into from
Apr 5, 2024
Merged

Fix user executor's requests from being blocked #1398

merged 5 commits into from
Apr 5, 2024

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Apr 5, 2024 •
edited
Loading

Description

One Line Summary

Drop any cached Identify User requests that can never go through in order to prevent blocking of other pending requests and allow recovery.

Details

The logic of this PR is a small change in this commit: Drop insurmountable cached requests in the user executor

Motivation

An app developer through testing and usage of a debugging proxy ended up in a scenario where the SDK called login from an anonymous state, but is forever missing the onesignal ID to complete the Identify User call.

  • No user updates or fetches can be made without an OSID.
  • The Identify User request forever sits first in queue waiting for an OSID.
  • There is no pending Create User request before it that will update the missing OSID.
  • Calls to logout or login("another_user") don't work as they are added to the executor queue but the queue is still blocked by the first request.

Scope

  • Dropping Identify User requests that can never go through to fulfill pending user requests and allow recovery.
  • I considered other options but this was most preferable
  • Also missed some User Defaults persistence in the executors.
  • Add more logs.

Testing

Unit testing

None

Manual testing

Physical iPhone 13 with iOS 17.4

I was not able to naturally reproduce this state by a combination:

  • Making the anonymous user's create user call fail
  • Making the Identify User call fail

I reproduced this state by:

  1. The anonymous Create User call succeeds but don't hydrate which doesn't give the SDK an OSID
  2. Then call login("a"), then logout(), login("a"), logout(), login("a")
  3. See there are 5 pending requests in the User Executor, along with some other pending requests in other executors
  4. On the next cold start, those 5 pending requests are still there and nothing can move forward
  5. Make the changes in this PR
  6. On the next cold start, the first Identify User request is dropped, so that logout(), login("a"), logout(), login("a") are the pending requests
  7. Those go successfully and the SDK ends up in a state with the correct user "a" and correct data for user "a"

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
  • 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.
  • I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

@nan-li
[logs] - add more logs, clarify descriptions
ae0ad11 
Add more verbose logging
Make descriptions of objects clearer.
@nan-li
address that missing OSID on new sessions is "OK"
005a7b6 
* This was a todo and after thinking about it and testing, I do think this is unexpected but OK.
* Any remedy to this should not be happening right here anyway
@nan-li
Drop insurmountable cached requests in the user executor
c83dbb1 
* Context: The user executor stops executing requests from its queue once it hits a request that cannot be sent (yet) since these need to go in order and some requests may be waiting on information from the response to previous request.
* When the user executor uncaches any requests, such requests can be:
- Fetch Identity by Subscription - only requires an app_id to be sent
- Create User - only requires app_id to be sent
- Transfer Subscription - requires subscription_id to be sent
- Identify User - requires the OSID of the anonymous user to be sent

* Of those, we already drop transfer subscription requests who are missing the subscription_id and cannot be hooked up to a model to await the subscription _id. Otherwise, this request will forever block.
* In this commit, we add the Identify User request as droppable if it is missing an OSID and it cannot be hooked up to an existing identity model to await the OSID. Otherwise, this request will forever block and remedy calls to `logout` or `login` will just sit in the queue. This scenario leads the SDK to be in a state of having an EUID but forever no OSID, which is needed to make server calls.
@nan-li
Persist executor queues on start / init
5e81b3d 
* The executors read from cache but did not re-persist the new data if some requests or deltas were removed
@nan-li
[nit] change a todo to a clarifying comment
efef827
@nan-li nan-li merged commit 21bda18 into main Apr 5, 2024
4 checks passed
@nan-li nan-li deleted the fix_user_executor_cache_from_blocking branch April 5, 2024 21:57
This was referenced Apr 5, 2024
Merged
Merged
This was referenced Apr 10, 2024
Merged
Merged
This was referenced Apr 10, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkasten2 jkasten2 jkasten2 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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