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

Use Duration as the ground truth for communicating durations #4256

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

dkhalanskyjb
Copy link
Collaborator

Historically, the library evolved using "a Long of milliseconds" as the standard of denoting durations.
Since then, kotlin.time.Duration appeared, encompassing a number of useful conversions.

There are several consequences to this change.

  • Before, delay(Long) and delay(Duration) were not easily expressed via one another. For example, delay(Long.MAX_VALUE / 2 + 1) (up until Long.MAX_VALUE) used to be considered a valid delay, but it was not expressible in delay(Duration). Therefore, delay(Long) was the more fundamental implementation. However, delay(Duration) could not just be expressed as delay(inWholeMilliseconds), as we need to round the durations up when delaying events, and this required complex logic. With this change, delay(Duration) is taken as the standard implementation, and delay(Long) is just delay(timeMillis.milliseconds), simplifying the conceptual space.
  • The same goes for other APIs accepting either a duration or some Long number of milliseconds.
  • In several platform APIs, we are actually able to pass nanoseconds as the duration to wait for. We can now accurately do that. This precision is unlikely to be important in practice, but it is still nice that we are not losing any information in transit.
  • On Android's main thread, it's no longer possible to wait for Long.MAX_VALUE / 2 milliseconds: it's considered an infinite duration. Long.MAX_VALUE / 2 - 1 is still fine.
  • In kotlinx-coroutines-test, before, it was possible to observe correct behavior for up to Long.MAX_VALUE milliseconds. Now, this value is drastically reduced, to be able to test the nanosecond precision.
  • In kotlinx-coroutines-test, we now fail with an IllegalStateException if we enter the representable ceiling of time during the test. Before, we used to continue the test execution, only using the order in which tasks arrived but not their virtual time values.

Historically, the library evolved using "a Long of milliseconds"
as the standard of denoting durations.
Since then, `kotlin.time.Duration` appeared, encompassing a number
of useful conversions.

There are several consequences to this change.
- Before, `delay(Long)` and `delay(Duration)` were not easily
  expressed via one another.
  For example, `delay(Long.MAX_VALUE / 2 + 1)`
  (up until `Long.MAX_VALUE`) used to be considered a valid
  delay, but it was not expressible in `delay(Duration)`.
  Therefore, `delay(Long)` was the more fundamental implementation.
  However, `delay(Duration)` could not just be expressed as
  `delay(inWholeMilliseconds)`, as we need to round the durations
  up when delaying events, and this required complex logic.
  With this change, `delay(Duration)` is taken as the standard
  implementation, and `delay(Long)` is just
  `delay(timeMillis.milliseconds)`, simplifying the conceptual
  space.
- The same goes for other APIs accepting either a duration or
  some Long number of milliseconds.
- In several platform APIs, we are actually able to
  pass nanoseconds as the duration to wait for.
  We can now accurately do that. This precision is unlikely to
  be important in practice, but it is still nice that we are
  not losing any information in transit.
- On Android's main thread, it's no longer possible to wait for
  `Long.MAX_VALUE / 2` milliseconds: it's considered an infinite
  duration.
  `Long.MAX_VALUE / 2 - 1` is still fine.
- In `kotlinx-coroutines-test`, before, it was possible to
  observe correct behavior for up to `Long.MAX_VALUE` milliseconds.
  Now, this value is drastically reduced, to be able to test
  the nanosecond precision.
- In `kotlinx-coroutines-test`, we now fail with
  an `IllegalStateException` if we enter the
  representable ceiling of time during the test.
  Before, we used to continue the test execution, only using the
  order in which tasks arrived but not their virtual time values.
@dkhalanskyjb
Copy link
Collaborator Author

dkhalanskyjb commented Oct 23, 2024

It's a draft to signify that I'm not sure this change is needed and want a second opinion. The simplified code looks nicer to me, but other than that, it's technically a breaking change without a clear user need. I mostly did this to thoroughly familiarize myself with time-based operations in our library.

Copy link
Member

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

I am generally okay with the approach (probably we'd like to target a major release here).

Maybe we should also align it with all-compatibility and provide a migration window just in case.

Note that I'd rather skimmed through than reviewed carefully, will do when you'll promote it to the PR

@kevincianfarini
Copy link
Contributor

Relates to #3920 and specifically this issue comment. I really like this change and that it theoretically allows for delays to be more granular than milliseconds.

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.

3 participants