std::time inconsistencies

[debris]

When investigating one of the issues in parity-ethereum repo I noticed several inconsistencies in std::time.

I prepared two short snippets

snippet A

use std::time::{Instant, Duration};

fn main() {
    let now = Instant::now();
    let max_nanoseconds = u64::max_value() / 1_000_000_000;
    let duration = Duration::new(max_nanoseconds, 0);
    println!("{:?}", now + duration);
}

snippet B

use std::time::{Instant, Duration};

fn main() {
    let now = Instant::now();
    let max_nanoseconds = u64::max_value() / 1_000_000_000 + 1;
    let duration = Duration::new(max_nanoseconds, 0);
    println!("{:?}", now + duration);
}

Both of the attached snippets are properly executed on linux (gist), but they panic on macos.

snippet A panics with 'overflow when adding duration to instant'

snippet B panics with 'overflow converting duration to nanoseconds'

---

This is an inconsistent behaviour of standard library and I believe that above snippets should panic (or work) on all platforms in the same way

[sfackler]

Instant is a wrapper around the most precise available time source on each OS. Those sources differ in the size of their supported ranges but you should generally be able to assume they'll work with within a range of 100 or so years.

[vitvakatu]

Moreover, we've run into this issue on Windows too.
The following test passes on Linux, but fails on Windows:

let times = [
            UNIX_EPOCH,
            UNIX_EPOCH + Duration::new(13, 23),
            SystemTime::now(),
            SystemTime::now() + Duration::new(17, 15),
            UNIX_EPOCH + Duration::new(0, u32::max_value()),
            UNIX_EPOCH + Duration::new(i64::max_value() as u64, 0),
            UNIX_EPOCH + Duration::new(i64::max_value() as u64, 999_999_999),
            UNIX_EPOCH + Duration::new(i64::max_value() as u64 - 1, 1_000_000_000),
            UNIX_EPOCH + Duration::new(i64::max_value() as u64 - 4, 4_000_000_000),
            UNIX_EPOCH + Duration::new(i64::max_value() as u64 - 4, u32::max_value()),
        ];

I also think that the behavior must be consistent on all three main platforms, at least.

This issue might be related: #44394

[sfackler]

SystemTime is also a wrapper around the underlying system type. They do not all support times that are 300 billion years in the future.

[vitvakatu]

@sfackler I see your point, but what we're asking for, is to make behaviour consistent on every platform.

[retep998]

The behavior can't be consistent across platforms, because this is inherently platform specific behavior. Different platforms support different ranges and have different precisions.

[varkor]

At the very least, this ought to be better documented. There aren't any explicit warnings that std::time is platform-dependent (apart from the name SystemTime specifically, I don't think this is ever obvious).

On the other hand, unless the documentation regarding Duration is modified from "A Duration type to represent a span of time, typically used for system timeouts." to "A Duration type representing a span of system time." (or equivalent), I don't see why this restriction should be made — people are going to use it for non-system-related tasks at the moment, and as such the inconsistent behaviour is unexpected and poorly abstracted, even if they are in corner cases.

[retep998]

Duration itself is platform independent. It's just Instant and SystemTime which are platform specific, and adding (or subtracting) a Duration to an Instant or SystemTime can cause overflow on some platforms and not others.

But yes, better documentation is a good idea.

[eira-fransham]

This appears to be because on non-mac unix systems Instant is stored as seconds and nanoseconds (independently) whereas on on macOS it's stored as a u64, representing nanos since the unix epoch. A workaround for this would be to use SystemTime instead, which uses stores seconds and nanoseconds seperately on all platforms.

[vitvakatu]

@Vurich no, SystemTime behaves different on different platforms as well.

[kstrafe]

@retep007 encountered this error recently. When doing pure time calculations one would expect that adding a nanosecond would give a duration of 1 nanosecond.

(x + Duration::from_nanos(1)).duration_since(x)

does not return a nanosecond on windows.

Is there any argument for not making the Instant nanosecond-precise regardless of platform and just truncate it when doing syscalls?

[hrvolapeter]

@retep998 one for you ☝️ 😅

[Scarjit]

I'm not sure, if my problem is worth it's own issue:
When using std::time::Duration, the as_nanos function returns an u128.
However it is impossible to import it, without casting to u64, since from_nanos has no u128 overload.

[ririsoft]

I encountered something strange on Windows 10. In an unittest with the following code

let t = Instant::now() - Duration::from_secs(3600);

I get some overflow when subtracting duration from instant error, in an unreproducible way : sometimes it works for days, and sometimes it keeps failing until I change the duration down to 1800 seconds. Putting it back to 3600 makes the test fails again, the only way to avoid the overflow is to change the code to :

let d = Duration::from_secs(3600);
let mut now = Instant::now();
now -= d;
let t = now;

After that I can move my code back to

let t = Instant::now() - Duration::from_secs(3600);

and it starts working again.

I am really wondering what is going on compiler and/or OS side to have such behavior.

[mati865]

@ririsoft Instant::now() provides time since something (at least on Windows it seems to be time since boot), you can verify it by using dbg! macro.
Then you are subtracting 1 hour from that. On Windows Instant cannot be in the past (in other words have negative value) but on Linux it's allowed.

This is platform specific limitation and there is not much that can be done about it.

[ririsoft]

at least on Windows it seems to be time since boot

This is it, thanks a bunch. For my use case SystemTime better suite then.

[poliorcetics]

I can add the documentation about the system-specificities but I'm not exactly clear on where it should go: std::time or the relevant types ? I'm in favour of the former, Duration is not system-dependent and can still be used to force a panic but I would like confirmation.