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

Consider removing Clock.asTimeSource #372

Open
dkhalanskyjb opened this issue Mar 21, 2024 · 12 comments · May be fixed by #450
Open

Consider removing Clock.asTimeSource #372

dkhalanskyjb opened this issue Mar 21, 2024 · 12 comments · May be fixed by #450
Labels
breaking change This could break existing code
Milestone

Comments

@dkhalanskyjb
Copy link
Collaborator

There are no known public usages of this function: https://grep.app/search?q=asTimeSource&words=true Also, Clock.System.asTimeSource().measureTime { } looks fairly benign, even though it doesn't do what you want, given that the system clock can jump in time by several minutes occasionally.

@mgroth0
Copy link

mgroth0 commented Mar 21, 2024

What's the purpose of it then? What can it do that Monotonic cannot?

@dkhalanskyjb
Copy link
Collaborator Author

What's the purpose of what? Clock? It's to show you the current time, according to the system. 2024-03-22T09:43:15Z, as of writing.

@mgroth0
Copy link

mgroth0 commented Mar 22, 2024

Sorry, I meant what is the purpose of Clock.System.asTimeSource() as opposed to using Monotonic. Both are instances of TimeSource, but the Monotonic kdoc says it is the "most precise time source available in the platform". So I just asked out of curisosity if there is any use imaginable use case for Clock.System.asTimeSource(), since you suggest removing it.

@dkhalanskyjb
Copy link
Collaborator Author

The purpose could be, for example, to define your own Clock (not Clock.System) for testing and then also use it as a TimeSource to synchronize the understanding of the time between different parts of your application.

@hfhbd
Copy link
Contributor

hfhbd commented Apr 25, 2024

How is this issue linked to #382?

Also, isn't the (wall) clock based Timesource more precise than the monotonic due to ntp syncs/monotonic pauses, see #164 (comment)?

@dkhalanskyjb
Copy link
Collaborator Author

dkhalanskyjb commented Apr 25, 2024

How is this issue linked to #382?

If we decide this function is useful after all, I think that it, too, should migrate to the stdlib.

Also, isn't the (wall) clock based Timesource more precise than the monotonic due to ntp syncs/monotonic pauses, see #164 (comment)?

Wall clocks are neither more nor less precise than TimeSource.Monotonic, they simply serve different needs and fail in different scenarios.

  • TimeSource.Monotonic measures the elapsed time from the point of view of a program. If we start now, let the program run for a couple of seconds, the time measured by TimeSource.Monotonic will be two seconds. It won't be -2 seconds, it won't be 0. You don't know what the wall clock will say, because someone could have adjusted it in the meantime, or because the computer didn't consider some amount of time due to being hibernated.
  • Clock.System.now() finds out the current wall-clock time, according to the computer. We could look at the clock now, and two seconds later (in terms of the time that actually elapsed), the wall clock could stay the same, or even shift to an earlier time, or whatever, because someone adjusted the clock in the meantime.

@dkhalanskyjb
Copy link
Collaborator Author

dkhalanskyjb commented Apr 25, 2024

Unrelated to the discussion, but a note about something that made me curious:

because the computer didn't consider some amount of time due to being hibernated.

Looks like this isn't actually what's going to happen with TimeSource.Monotonic: internally, it delegates to https://en.cppreference.com/w/cpp/chrono/steady_clock (https://github.com/JetBrains/kotlin/blob/d00030674aa8323f945fda8d23d55480c1868ba3/kotlin-native/runtime/src/main/cpp/Porting.cpp#L258-L267, https://github.com/JetBrains/kotlin/blob/d00030674aa8323f945fda8d23d55480c1868ba3/kotlin-native/runtime/src/main/kotlin/kotlin/time/MonotonicTimeSource.kt#L16), which promises that "the time between ticks of this clock is constant." If we take this at face value to mean "real time" and not things like "CPU time", hibernation shouldn't affect this.

@dalewking
Copy link

Not a public usage of this, but our project uses this for one reason. We have a cache class that is supposed to expire. When we store a value we also store a TimeMark with it to know whether the value has expired. That time mark comes from an object that implements Clock so that we can control time in tests to tests things like the cache expiration.

I think the problem with Monotonic is that it doesn't have a good way to control time in tests. It could probably be made to do so, but I think it is not quite there.

@dkhalanskyjb
Copy link
Collaborator Author

I think the problem with Monotonic is that it doesn't have a good way to control time in tests.

Did you try https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.time/-test-time-source/ ? What's missing from it?

@hfhbd
Copy link
Contributor

hfhbd commented May 7, 2024

I think the problem with Monotonic is that it doesn't have a good way to control time in tests.

What about TestTimeSource?

@dkhalanskyjb dkhalanskyjb added the breaking change This could break existing code label Oct 15, 2024
dkhalanskyjb added a commit that referenced this issue Oct 18, 2024
@dkhalanskyjb dkhalanskyjb linked a pull request Oct 18, 2024 that will close this issue
@ilya-g
Copy link
Member

ilya-g commented Oct 29, 2024

That time mark comes from an object that implements Clock so that we can control time in tests to tests things like the cache expiration.

@dalewking However in production, do you use Monotonic time source or the one that is obtained from Clock.System as well? I.e. are your cache expiration timeouts tied to the wall clock time or monotonic time?

@dalewking
Copy link

@dalewking However in production, do you use Monotonic time source or the one that is obtained from Clock.System as well? I.e. are your cache expiration timeouts tied to the wall clock time or monotonic time?

Just now getting back to this and we were not using Monotonic and now I agree that Clock.asTimeSource should go away.

For a good discussion on the topic see: https://www.youtube.com/watch?v=i1wysckHg2o

@dkhalanskyjb dkhalanskyjb added this to the 0.9.0 milestone Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This could break existing code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants