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.

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

[Feat] Add backoff to OperationRepo when retrying network calls #2017

Merged
merged 1 commit into from
Mar 7, 2024

Conversation

jkasten2
Copy link
Member

@jkasten2 jkasten2 commented Mar 5, 2024

Description

One Line Summary

Add a delay to retries with a backoff to slow down network calls from OperationRepo.

Details

Motivation

Retrying too quickly:

  • puts unnecessary load on the device and OneSignal's backend.
  • may cause multiple OneSignal Users being created
    • some of them being in a bad state with the same external_id but different onesignal_id values due to this fast retrying.
    • Mostly only an issue when OperationRepo.force is true, which causes the 5 second batching to delay to be skipped, which normally get set when calling OneSignal.login

Scope

Only affects adding a delay to retries on OperationRepo.

Testing

Unit testing

To ensure this works as expected we updated the retrying test to confirm the delay is called.

Manual testing

Also to ensure this commit works as intended I ran this on an Android 14 emu and ensured it waits the right amount of time when there is 500 response. Also after we stop getting a 500 error the queue successfully resumes and all pending tags are even added too. While it's waiting for the next retry I ensured it didn't lock main thread as well.

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

If we got a 5xx response from the OneSignal backend we were retrying
immediately. This result wasn't positive for the end-user, device,
or OneSignal's backend.

To ensure this works as expected we updated the retrying test to
confirm the delay is called.

Also to ensure this commit works as intended I ran this on an Android 14
emu and ensured it waits the right amount of time when there is 500
response. Also after we stop getting a 500 error the queue successfully
resumes and all pending tags are even added too.
Copy link
Contributor

@nan-li nan-li left a comment

Choose a reason for hiding this comment

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

LGTM

  • In my testing, without this PR, retries happen every 5 seconds, so they weren't so immediate as is.
  • I do think there are some code paths that do because I have seen my logcat spamming retries in the past.

@jkasten2
Copy link
Member Author

jkasten2 commented Mar 7, 2024

@nan-li Thanks for the review!

  • In my testing, without this PR, retries happen every 5 seconds, so they weren't so immediate as is.
  • I do think there are some code paths that do because I have seen my logcat spamming retries in the past.

In my testing there are cases where OperationRepo.force gets set to true and this skips the 5 second batching logic. This normally happens when OneSignal.login is called.

  • I updated the PR description to include this too

@jkasten2 jkasten2 merged commit c371096 into main Mar 7, 2024
2 checks passed
@jkasten2 jkasten2 deleted the feat/add_backoff_to_operation_retries branch March 7, 2024 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants