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

Subtracting Duration from Instant on Mac is panic-prone #100141

Open
matklad opened this issue Aug 4, 2022 · 4 comments
Open

Subtracting Duration from Instant on Mac is panic-prone #100141

matklad opened this issue Aug 4, 2022 · 4 comments
Labels
C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@matklad
Copy link
Member

matklad commented Aug 4, 2022

The following program:

fn main() {
    let t = std::time::Instant::now();
    let d = std::time::Duration::from_secs(60);
    eprintln!("{:?}", (t - d).elapsed());
}

works on Linux, but could panic on Mac. The reason for that is that, as implemented, Instant on mac starts at zero at reboot, so it can't represent times before the machine started.

This is pretty unfortunate -- it means that impl Sub<Duration> for Instant isn't really usable on mac, as subtracting even a small duration can panic. And subtracing duration is pretty useful and natural too implement tasks like "periodically, cleanup data crated earlier than Instant::now() - Duration::from_secs(60)".

While technically we don't give any guarantees on Instant range and document this as system-dependent, it is really unfortunate that subtracting API exists, works fine on Linux, and fails occasionally on macs. I think we should just go and patch the time on our side, to get some sort of a reasonable range bothways.

I think just

diff --git a/library/std/src/sys/unix/time.rs b/library/std/src/sys/unix/time.rs
index f99c453a3a8..994320564c4 100644
--- a/library/std/src/sys/unix/time.rs
+++ b/library/std/src/sys/unix/time.rs
@@ -168,7 +168,7 @@ pub fn now() -> Instant {
             extern "C" {
                 fn mach_absolute_time() -> u64;
             }
-            Instant { t: unsafe { mach_absolute_time() } }
+            Instant { t: unsafe { mach_absolute_time() } + u64::MAX / 2 }
         }
 
         pub fn checked_sub_instant(&self, other: &Instant) -> Option<Duration> {

would do the trick, but I am wondering if there's a better way to do this.

Manual for mach_absolute_time (https://developer.apple.com/documentation/kernel/1462446-mach_absolute_time) also suggests using clock_gettime_nsec_np(CLOCK_UPTIME_RAW)instead, so we can perhaps switch to that while we are at it.

@matklad matklad added the C-bug Category: This is a bug. label Aug 4, 2022
@nmain
Copy link

nmain commented Aug 4, 2022

Is the situation on Windows also concerning? It uses QueryPerformanceCounter which doesn't guarantee a particular epoch.

@ChrisDenton
Copy link
Contributor

To be honest, I think I agree that Add<Duration> and Sub<Duration> should probably not have been implemented for Instant. It doesn't make sense given the current documentation (and implementations) of Instant as they can only really be compared with other Instants.

That said, there is definitely a use case for adding/subtracting precise times. But couldn't that be satisfied by SystemTime? Maybe with a new method if necessary.

@the8472
Copy link
Member

the8472 commented Aug 4, 2022

And subtracing duration is pretty useful and natural too implement tasks like "periodically, cleanup data crated earlier than Instant::now() - Duration::from_secs(60)".

You can do this instead: foo.created_at + Duration::from_secs(60) < Instant::now()

@dimo414
Copy link
Contributor

dimo414 commented Nov 27, 2023

This is really surprising behavior. Instant is basically impossible to use safely given the existing implementation on OSX etc. It's understandable (if unfortunate) that arithmetic breaks down in platform-specific ways when operating on times in the distant past/future, but if you can't reliably construct an instant that's just a few minutes older than now() the entire type is fundamentally unsafe to use.

If Add/Sub on Instant are in fact mis-features it would be helpful to deprecate their use and document the intended alternatives. It's not at all obvious to me what someone's supposed to use instead of Instant if they want to refer to a moment in time but avoid the limitations of SystemTime (who's documentation recommends using Instant for safe elapsed time calculations), so clear documentation would be very helpful.

dimo414 added a commit to dimo414/bkt that referenced this issue Jan 13, 2024
Although monotonic and therefore safe from certain failure modes that
SystemTime arithmetic can encounter, Instant's implementation is not
consistent and can panic in certain implementations
(rust-lang/rust#100141).

Despite the risk of errors I've also (mostly) convinced myself that
SystemTime is the more-correct type to use for this purpose, and my
attempts to avoid it by converting them to Instants are mostly unhelpful
(e.g. if the system time has changed the Instant representations would
be wrong too).

This PR includes a breaking API change to the `CacheStatus` enum, but
should not affect CLI users.
dimo414 added a commit to dimo414/bkt that referenced this issue Jan 13, 2024
Although monotonic and therefore safe from certain failure modes that
SystemTime arithmetic can encounter, Instant's implementation is not
consistent and can panic in certain implementations
(rust-lang/rust#100141).

Despite the risk of errors I've also (mostly) convinced myself that
SystemTime is the more-correct type to use for this purpose, and my
attempts to avoid it by converting them to Instants are mostly unhelpful
(e.g. if the system time has changed the Instant representations would
be wrong too).

This PR includes a breaking API change to the `CacheStatus` enum, but
should not affect CLI users.

Fixes #45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants