-
Notifications
You must be signed in to change notification settings - Fork 372
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
Fix ending an already ended session #2185
Fix ending an already ended session #2185
Conversation
# What Fixes an issue where invalid REST API requests were made to both outcomes/measure and User updates with session_time were bing made in the background. This is only present when location sharing is enabled in the SDK (in addition to the app and end-user allowing it) and the end-user opens the app at any point after receiving a notification. This resulted in 400 errors in the outcomes/measure case and no-op User updates that wasting resources, the later doesn't depend on receiving notifications. The outcomes/measure is also additive as the bad requests are cached and retried on app open. # How Add a state check to SessionService to prevent ending an already ended session. Also address a previous session never ending correctly when the app is cold started. Lastly, added comments pointing out the caveats of IBackgroundService to help prevent future wrong assumptions about when backgroundRun() is run. # Follow up Since there are cached invalid outcomes/measure requests a migration needs to be created in the SDK to remove them for apps who were affected by the bug.
Clean up invalid cached os__session_duration outcome records with zero session_time produced in SDK versions 5.1.15 to 5.1.20 so we stop sending these requests to the backend.
ca46e6f
to
1ab8dcc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good and the changed comments are proper. I have also played with the test unit to verify that the results are correct.
I just have one question that since isValid is false by default and will set to true after the endSession call in onFocus(), is it expected that the first endSession() call after the very first app focus will always return early and do nothing?
Please disregard the question here as it is now clear to me that the change is necessary to start a new session if app is cold started. |
Description
One Line Summary
Fixes invalid REST API requests, when location is on, to both outcomes/measure and User updates with
session_time
being zero/missing in the background.Details
This is only present when location sharing is enabled in the SDK (in addition to the app and end-user allowing it) and the end-user opens the app at any point after receiving a notification. This resulted in 400 errors in the outcomes/measure case and no-op User updates that wasting resources, the later doesn't depend on receiving notifications. The outcomes/measure is also additive as the bad requests are cached and retried on app open.
Motivation
These invalid and no op request put a lot of extra load on both the device and the OneSignal backend.
Scope
Session tracking only.
Testing
Unit testing
Added a new session test and update an existing one.
Manual testing
Tested on an Android 14 emulator with our example app.
Reproducing the issue:
os__session_duration
with no session_time sent and the 400 response.Reproducing repeating bad cached values:
session_time
being relayed.Affected code checklist
Checklist
Overview
Testing
Final pass
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)