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

Remove initialization state from the SNTP service #51

Merged
merged 1 commit into from
Nov 23, 2020
Merged

Remove initialization state from the SNTP service #51

merged 1 commit into from
Nov 23, 2020

Conversation

arturdryomov
Copy link
Member

Upstreaming an internal change authored by @buildbreaker (internal #60231).

The app does a sync almost immediately after app launch. The state is almost always set to SYNCING before the get is called. This means that the INIT comparison is always false so the boot time check will never be triggered.

This should fix a large class of issues related to NTP. Specifically, if the user restarts their phone and then opens the app in poor network connectivity, they'll likely fail all the NTP requests and will be stuck using the persisted NTP time. Since the user restarted their app, the calculations for response age would be completely off (since the current elapsed ticks would always be lower than the elapsed ticks in the persistence).

Finally, we only do NTP syncs on an app open so if this were to trigger on a specific session, the rest of the session's timestamps would be off too.

PTAL @buildbreaker, @ameliariely, @amphora001

Copy link

@buildbreaker buildbreaker left a comment

Choose a reason for hiding this comment

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

Nice! Thank you for upstreaming this change!

@buildbreaker buildbreaker merged commit 21fa5b8 into lyft:master Nov 23, 2020
@arturdryomov arturdryomov deleted the ad/sntp-state branch November 23, 2020 18:35
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