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.

Sign up for GitHub

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

Jump to bottom

getCurrentTimeMs() return the wrong date time everytime I manually change the device clock #61

Open
robsonbbs opened this issue Oct 19, 2021 · 10 comments · May be fixed by #63
Open

getCurrentTimeMs() return the wrong date time everytime I manually change the device clock #61

robsonbbs opened this issue Oct 19, 2021 · 10 comments · May be fixed by #63

Comments

@robsonbbs
Copy link

robsonbbs commented Oct 19, 2021 •
edited
Loading

If I do something like:

  • Open my app
  • Minimize it and go to System > Date & Time and then change the device clock for some wrong date & time
  • Then come back to my app and try to call getCurrentTimeMs() this will give me the wrong date that I manually setup

If I have an internet connection, after that wrong response the next one works as expected, giving me the right date & time.

If that is expected behavior could you please explain a little bit more why this happens? And what do you recommend to minimize that behavior?

I am now calling kronosClock.syncInBackground(); every time my app goes to the foreground, please let me know if there is any other better strategy to deal with it.

Thank you in advance, and congrats for the work, apart from that minor issue this library is great!
@rluick15
Copy link

rluick15 commented Jan 7, 2022

Im actually now running into this as well. The system time is returned immediately after updating the date manually. It the resets after a sync

@jh0719
Copy link

jh0719 commented Feb 3, 2022

I am also experiencing strange behavior with this testing flow. kronos will have a valid NTP time and return it to me. When I minimize and manually set phone time, and return to my app, kronos appears to have cleared the NTP time offsets for some reason, and now I am being returned the incorrect phone time.

I expected the whole point of this library was to maintain the offset between RTC clock and NTP time and thus still be able to give me NTP time.

@robsonbbs
Copy link
Author

Guys, if it helps: My app is running for a while with the workaround that I suggested in my previous comment (call kronosClock.syncInBackground(); when the app goes to foreground), I don't think is a definitive solution, but works and looks like is what we have for now.

@jh0719
Copy link

jh0719 commented Feb 7, 2022

I don't think it works since we're testing scenarios where there's no internet and syncInBackground would fail.

The scenario we're trying to work for:

  1. Phone has internet, app is launched, kronos syncs with server and gets time.
  2. User turns off internet on phone (and mobile data)
  3. User manually changes phone time to something else. Phone is NOT rebooted

Expectation:
Kronos still knows and returns NTP time, since it should have an offset of RTC clock to Server time that is valid.

Reality:
Kronos does not know or return NTP time since it for some reason flushed the caches.

@robsonbbs
Copy link
Author

Yes, that's why I don't think this is a definitive solution, is just a workaround that reduces this issue impact.

@robsonbbs
Copy link
Author

Maybe @keith @arturdryomov or someone else at Lyft team could guide us into a definitive solution.

@Rajin9601
Copy link

Rajin9601 commented Feb 11, 2022 •
edited
Loading

I think the problem is related to the way that kronos-android detects whether the system has been rebooted.

Kronos-Android/kronos-java/src/main/java/com/lyft/kronos/internal/ntp/SntpClient.java

Lines 260 to 269 in b2774ab

/**
* @return True if the system has not been rebooted since this was created, false otherwise.
*/
boolean isFromSameBoot() {
final long bootTime = this.deviceCurrentTimestampMs - this.deviceElapsedTimestampMs;
final long systemCurrentTimeMs = deviceClock.getCurrentTimeMs();
final long systemElapsedTimeMs = deviceClock.getElapsedTimeMs();
final long systemBootTime = systemCurrentTimeMs - systemElapsedTimeMs;
return Math.abs(bootTime - systemBootTime) < MAX_BOOT_MISMATCH_MS;
}

The library uses deviceElapsedTimestampMs from SystemClock in android as the source of local time.
However, this value will be reset when the system is booted. So library needs to know whether the system has been rebooted since it got NTP time from the servers.

To detect the reboot, library uses difference between deviceCurrentTimeMillis and deviceElapsedTime. If the diff has changed significantly, it assumes that the device is rebooted. So if you change the device clock and open the app, the diff will change and library thinks that the device is rebooted and throw away the cache. (Which is ok since most users don't change the system clock & even when they do, kronos will try to synchronize and eventually get the NTP time)

I cannot think of other way of detecting boot without changing the Clock interface in kronos.
I think using Settings.Global#BOOT_COUNT will fix this problem.

@Rajin9601 Rajin9601 linked a pull request Feb 11, 2022 that will close this issue
Open
@jh0719
Copy link

jh0719 commented Feb 11, 2022 •
edited
Loading

@Rajin9601 That's a good find.

I don't think I agree that it's a correct behavior. We have an additional tool available to us to know if the phone was rebooted vs the time being altered manually by the user. Basically, it's the runtime (or the existence of memory).

If the realtime clock to currentTimeMillis offset (or difference) is changed or invalidated, but the library still exists in memory or runtime (I mean the app is still running), then we know the device wasn't rebooted. Since we know this, we know that the offset between realtime clock and NTP time is still valid, and thus we can still have a good state of "NTP time certainty".

Now if the app running the kronos library is killed, and then the user changes phone time, well, we're out of luck. THere's no way to distinguish if the phone rebooted from user phone time.

I guess I'm wondering why the library doesn't simply just rely on the offset maintained in memory? It seems to be clearing it's memory when the realtime and currentTimeMIllis offsets are invalidated, and that's unnecessary.

@jh0719
Copy link

jh0719 commented Feb 11, 2022

Although if your PR works well and gets approval, my suggestion is redundant.

@Rajin9601
Copy link

Rajin9601 commented Feb 14, 2022 •
edited
Loading

@jh0719 I think your suggestion (checking the reboot when getting the offset from the device cache, not when getting it from the memory) can also work.

Meanwhile, you can use broadcast receiver and re-sync in background when user changes the time. (code from the demo app)

Kronos-Android/app/src/main/java/com/lyft/kronos/demo/MainActivity.kt

Lines 50 to 67 in b2774ab

private val timeSettingsChangedReciever = object : BroadcastReceiver() {
override fun onReceive(context: Context?, intent: Intent?) {
Log.d(TAG, "Date and time settings changed")
val app = applicationContext as DemoApplication
app.kronosClock.syncInBackground()
}
}
private fun registerReceiver() {
val filter = IntentFilter()
filter.addAction(Intent.ACTION_DATE_CHANGED)
filter.addAction(Intent.ACTION_TIME_CHANGED)
filter.addAction(Intent.ACTION_TIMEZONE_CHANGED)
applicationContext.registerReceiver(timeSettingsChangedReciever, filter)
Log.d(TAG, "Registered to receive date time settings changes")
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use boot count to detect reboot if it exists Rajin9601/Kronos-Android
4 participants
@rluick15 @Rajin9601 @robsonbbs @jh0719