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

Shouldn't TimeSpec::from_duration preserve monotonicity? #2479

Open
sirhcel opened this issue Sep 1, 2024 · 3 comments
Open

Shouldn't TimeSpec::from_duration preserve monotonicity? #2479

sirhcel opened this issue Sep 1, 2024 · 3 comments

Comments

@sirhcel
Copy link

sirhcel commented Sep 1, 2024

When creating a TimeSpec from a std::time::Duration with more than libc::time_t seconds, the resulting value might be negative and will no longer preserve monotonicity with respect to the input duration. This happens to the integer casting in TimeSpec::from_duration.

See this example on Rust Playground.

I learned about this behavior through errors for negative timeouts from ppoll on Linux at runtime (serialport/serialport-rs#207). I did not expect this for the "porcellain" type Duration and the docs did not hint at it. And likely other users of the "porcellain" as well. When manually converting to the "plumbing" typetime_t I wold have looked deeper into the nitty gritty integer casts and cared for this case by saturating.

So shouldn't TimeSpec::from_duration preserve monotonicity? Isn't an inaccurate but still large TimeSpec less surprising?

@asomers
Copy link
Member

asomers commented Sep 2, 2024

The problem is that a Unix Timespec can't represent times greater than i64::MAX seconds. Saturating the conversion would be bad, because then subtracting two TimeSpecs that were produced by TimeSpec::from_duration wouldn't be guaranteed to give the correct answer. Arguably, what Nix ought to do would be to provide TimeSpec::from_instant(i: std::time::Instant) instead of from_duration, because std::time::Instant actually uses a TimeSpec under the hood. And perhaps a TryFrom<Duration> impl, too.

@sirhcel
Copy link
Author

sirhcel commented Sep 3, 2024

The problem is that a Unix Timespec can't represent times greater than i64::MAX seconds. Saturating the conversion would be bad, because then subtracting two TimeSpecs that were produced by TimeSpec::from_duration wouldn't be guaranteed to give the correct answer.

This is a fair point against saturating.

And perhaps a TryFrom<Duration> impl, too.

What about transitioning towards this impl over some time? I could prepare PRs for adding TryFrom<Duration> and once this is released the successor for deprecating From<Duration> so that other young players could get safely around this.

@asomers
Copy link
Member

asomers commented Sep 3, 2024

What about transitioning towards this impl over some time? I could prepare PRs for adding TryFrom<Duration> and once this is released the successor for deprecating From<Duration> so that other young players could get safely around this.

Yes, that would be good.

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

No branches or pull requests

2 participants