Skip to content

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

Merged
jkasten2 merged 1 commit intomainfrom
feat/add_backoff_to_operation_retries
Mar 7, 2024
Merged

[Feat] Add backoff to OperationRepo when retrying network calls#2017
jkasten2 merged 1 commit intomainfrom
feat/add_backoff_to_operation_retries

Conversation

@jkasten2
Copy link
Copy Markdown
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.
@jkasten2 jkasten2 requested review from emawby and nan-li March 5, 2024 23:16
Copy link
Copy Markdown
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
Copy Markdown
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

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