Time.kt uses Duration.toMillis in unsafe manner

[Groostav]

Using Duration.ofSeconds(Long.MAX_VALUE) as the timeout argument for kotlinx.coroutines.withTimeout will cause an ArithmaticOverflowException

This is because, in pure java Durations.ofSeconds(Long.MAX_VALUE).toMillis() will cause an Arithmatic overflow.

I've got a fix that basically involves adding an extension function toMillisOrMax that returns Long.MAX_VALUE when toMillis would otherwise overflow.

Use case:

I'm a big fan of null objects, and the way I've structured things is such that the "default" timeout is Duration.of(seconds = Long.MAX_VALUE, nanos = 999__999_999) (which is effectively Duration.MAX_VALUE), such that our down-stack code need not consider the case where no timeout is specified.

If you guys are interested in this fix I'll submit a pull request to that involves simply:

private fun Duration.toMillisOrMax(): Long = when(this.seconds) {
    in 0L .. (Long.MAX_VALUE / MILLIS_PER_SECOND) -> duration.toMillis()
    else -> Long.MAX_VALUE
}

as a function in TimeKt, invoked instead of Duration.toMillis()

[fvasco]

I simple workaround is to avoid time check at all if the duration exceed a large number.

So if the duration is greater than a century (example) then the code is executed without any timeout (this avoid some system calls and related work).

[qwwdfsad]

I simple workaround is to avoid time check at all if the duration exceeds a large number.
So if the duration is greater than a century (example) then the code is executed without any timeout (this avoid some system calls and related work).

The library should not do that. There are a lot of valid use-cases such as calling delay(MAX_VALUE) to rely on cancellation or withTimeout(MAX_VALUE) to rely on virtual time or something else (e.g. interference with child jobs and nested coroutines). Such behaviour ("do nothing if the timeout is greater than a century") is very cumbersome and unobvious and may interfere with thigs like NTP in a very obscure ways.

Not sure that Duration.toMillisOrMax is a good solution either, as it obviously break contract "throws TimeoutException if a specified timeout was exceeded"

[fvasco]

@qwwdfsad Do you consider coerce at most 292 million years each delay/timeout a good solution?
How this integration should handle the FOREVER Duration?

[avently]

Why not just use Long instead of this custom Duration? It breaks from migration to common code in the future.

[fvasco]

Long is a pure number, it miss of unit of measurement.

[avently]

Yes, but I didn't meet developer who doesn't know how to make minutes, hours, etc with Long.
So Duration class isn't a standart way to do things and it gives more sugar to your lib's code (converting from long to duration and back). Also it breaks some things in multiplatform projects. I forgot actual example, but I got someday classnotfoundexception because of duration

[JakeWharton]

Duration is actually the standard and long is the legacy mechanism.

[fvasco]

kotlinx-coroutines-jdk8 isn't a multiplatform project.

[avently]

Obviously. But I tried to migrate some java only code to common from ktor websockets (it has only java part now). One of the blockers was java.time package. That's why I think about migration from platform-specific to universal code in every project