Skip to content

[Improvement] Never set OperationRepo.enqueue's flush to true#2055

Merged
jkasten2 merged 1 commit intomainfrom
improve/op-repo-never-set-flush
Apr 15, 2024
Merged

[Improvement] Never set OperationRepo.enqueue's flush to true#2055
jkasten2 merged 1 commit intomainfrom
improve/op-repo-never-set-flush

Conversation

@jkasten2
Copy link
Copy Markdown
Member

@jkasten2 jkasten2 commented Apr 14, 2024

Description

One Line Summary

Never set OperationRepo.enqueue's flush to true to improve network batching.

Details

Motivation

The 5.x.x branch of the OneSignal SDK is not as efficient on it's network calls as 4.x.x was. This is one small change to help improve some of this.

Scope

Only effects delaying networks calls generated when calling OneSignal.login by up to seconds 5.

Testing

Manual testing

Tested calling OneSignal.login and ensured it was delayed by 5 seconds as expected.

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

OneSignal.login() was setting OperationRepo.enqueue to true, while this
means the OneSignal backend is updated up to 5 seconds faster it means
a possible increase in load, as not having a delay means we may not
have batched as many network requests.

There is a high chance we will re-introduce the flush via a new public
commit() method, so it wasn't remove in this commit. But before doing
so we need budget rules so it can't be abused.

One oddly I notice here is the login had a flush, but the logout did
not. This seems like a bug, but either way we don't want to flush
anywhere now.
@jinliu9508
Copy link
Copy Markdown
Contributor

Verified in my test that the login call is waited for more than 5 seconds in the queue to be processed.
image

However, I have two questions:

  1. Considering that both login() and logout() call LoginUserOperation, and several places call RefreshUserOperation, have we properly handled the case when the device or network is interrupted after the operation being enqueued but never executed?
  2. Now I am thinking about the old behavior regarding how adding Login/Refresh to the queue can affect other operations in the queue. Will setting the flush to true forcibly execute operations that are supposed to wait for the next interval, thereby causing some operations to be in a tight loop? If so, this PR may possibly solve that issue.

@jkasten2
Copy link
Copy Markdown
Member Author

jkasten2 commented Apr 15, 2024

  1. Considering that both login() and logout() call LoginUserOperation, and several places call RefreshUserOperation, have we properly handled the case when the device or network is interrupted after the operation being enqueued but never executed?

Yes, these operations are stored locally when enqueued, that way if the app is killed they are not lost and the queue can be resumed when the app started again.

  1. Now I am thinking about the old behavior regarding how adding Login/Refresh to the queue can affect other operations in the queue. Will setting the flush to true forcibly execute operations that are supposed to wait for the next interval, thereby causing some operations to be in a tight loop? If so, this PR may possibly solve that issue.

Removing the use of this flush flag means that the starting delay won't be skipped, so more operations could be grouped together after this PR. On the 404 issue, I believe this should help slow down (by 5 seconds) the tight loop (with the introduction of the bucket concept) but not fix the looping of create User itself.

Base automatically changed from fix/op-repo-add-bucketing-to-limit-network-calls-further to main April 15, 2024 20:31
@jkasten2 jkasten2 merged commit 9ad9885 into main Apr 15, 2024
@jkasten2 jkasten2 deleted the improve/op-repo-never-set-flush branch April 15, 2024 20:31
@jkasten2 jkasten2 mentioned this pull request Apr 15, 2024
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