Skip to content

Commit

Permalink
std: Force Instant::now() to be monotonic
Browse files Browse the repository at this point in the history
This commit is an attempt to force `Instant::now` to be monotonic
through any means possible. We tried relying on OS/hardware/clock
implementations, but those seem buggy enough that we can't rely on them
in practice. This commit implements the same hammer Firefox recently
implemented (noted in #56612) which is to just keep whatever the lastest
`Instant::now()` return value was in memory, returning that instead of
the OS looks like it's moving backwards.

Closes #48514
Closes #49281
cc #51648
cc #56560
Closes #56612
Closes #56940
  • Loading branch information
alexcrichton committed Jan 7, 2019
1 parent 1f7c44c commit 255a3f3
Show file tree
Hide file tree
Showing 7 changed files with 103 additions and 28 deletions.
17 changes: 2 additions & 15 deletions src/librustc/util/profiling.rs
Expand Up @@ -2,7 +2,7 @@ use session::config::Options;

use std::fs;
use std::io::{self, StderrLock, Write};
use std::time::{Duration, Instant};
use std::time::Instant;

macro_rules! define_categories {
($($name:ident,)*) => {
Expand Down Expand Up @@ -203,20 +203,7 @@ impl SelfProfiler {
}

fn stop_timer(&mut self) -> u64 {
let elapsed = if cfg!(windows) {
// On Windows, timers don't always appear to be monotonic (see #51648)
// which can lead to panics when calculating elapsed time.
// Work around this by testing to see if the current time is less than
// our recorded time, and if it is, just returning 0.
let now = Instant::now();
if self.current_timer >= now {
Duration::new(0, 0)
} else {
self.current_timer.elapsed()
}
} else {
self.current_timer.elapsed()
};
let elapsed = self.current_timer.elapsed();

self.current_timer = Instant::now();

Expand Down
8 changes: 8 additions & 0 deletions src/libstd/sys/cloudabi/time.rs
Expand Up @@ -25,6 +25,14 @@ impl Instant {
}
}

pub fn actually_monotonic() -> bool {
true
}

pub const fn zero() -> Instant {
Instant { t: 0 }
}

pub fn sub_instant(&self, other: &Instant) -> Duration {
let diff = self.t
.checked_sub(other.t)
Expand Down
8 changes: 8 additions & 0 deletions src/libstd/sys/redox/time.rs
Expand Up @@ -128,6 +128,14 @@ impl Instant {
Instant { t: now(syscall::CLOCK_MONOTONIC) }
}

pub const fn zero() -> Instant {
Instant { t: Timespec { t: syscall::TimeSpec { tv_sec: 0, tv_nsec: 0 } } }
}

pub fn actually_monotonic() -> bool {
false
}

pub fn sub_instant(&self, other: &Instant) -> Duration {
self.t.sub_timespec(&other.t).unwrap_or_else(|_| {
panic!("specified instant was later than self")
Expand Down
40 changes: 28 additions & 12 deletions src/libstd/sys/unix/time.rs
Expand Up @@ -14,6 +14,12 @@ struct Timespec {
}

impl Timespec {
const fn zero() -> Timespec {
Timespec {
t: libc::timespec { tv_sec: 0, tv_nsec: 0 },
}
}

fn sub_timespec(&self, other: &Timespec) -> Result<Duration, Duration> {
if self >= other {
Ok(if self.t.tv_nsec >= other.t.tv_nsec {
Expand Down Expand Up @@ -128,19 +134,22 @@ mod inner {
}

pub const UNIX_EPOCH: SystemTime = SystemTime {
t: Timespec {
t: libc::timespec {
tv_sec: 0,
tv_nsec: 0,
},
},
t: Timespec::zero(),
};

impl Instant {
pub fn now() -> Instant {
Instant { t: unsafe { libc::mach_absolute_time() } }
}

pub const fn zero() -> Instant {
Instant { t: 0 }
}

pub fn actually_monotonic() -> bool {
true
}

pub fn sub_instant(&self, other: &Instant) -> Duration {
let info = info();
let diff = self.t.checked_sub(other.t)
Expand Down Expand Up @@ -258,19 +267,26 @@ mod inner {
}

pub const UNIX_EPOCH: SystemTime = SystemTime {
t: Timespec {
t: libc::timespec {
tv_sec: 0,
tv_nsec: 0,
},
},
t: Timespec::zero(),
};

impl Instant {
pub fn now() -> Instant {
Instant { t: now(libc::CLOCK_MONOTONIC) }
}

pub const fn zero() -> Instant {
Instant {
t: Timespec::zero(),
}
}

pub fn actually_monotonic() -> bool {
(cfg!(target_os = "linux") && cfg!(target_arch = "x86_64")) ||
(cfg!(target_os = "linux") && cfg!(target_arch = "x86")) ||
false // last clause, used so `||` is always trailing above
}

pub fn sub_instant(&self, other: &Instant) -> Duration {
self.t.sub_timespec(&other.t).unwrap_or_else(|_| {
panic!("specified instant was later than self")
Expand Down
8 changes: 8 additions & 0 deletions src/libstd/sys/wasm/time.rs
Expand Up @@ -14,6 +14,14 @@ impl Instant {
Instant(TimeSysCall::perform(TimeClock::Monotonic))
}

pub const fn zero() -> Instant {
Instant(Duration::from_secs(0))
}

pub fn actually_monotonic() -> bool {
false
}

pub fn sub_instant(&self, other: &Instant) -> Duration {
self.0 - other.0
}
Expand Down
8 changes: 8 additions & 0 deletions src/libstd/sys/windows/time.rs
Expand Up @@ -40,6 +40,14 @@ impl Instant {
t
}

pub fn actually_monotonic() -> bool {
false
}

pub const fn zero() -> Instant {
Instant { t: 0 }
}

pub fn sub_instant(&self, other: &Instant) -> Duration {
// Values which are +- 1 need to be considered as basically the same
// units in time due to various measurement oddities, according to
Expand Down
42 changes: 41 additions & 1 deletion src/libstd/time.rs
Expand Up @@ -12,11 +12,13 @@

#![stable(feature = "time", since = "1.3.0")]

use cmp;
use error::Error;
use fmt;
use ops::{Add, Sub, AddAssign, SubAssign};
use sys::time;
use sys_common::FromInner;
use sys_common::mutex::Mutex;

#[stable(feature = "time", since = "1.3.0")]
pub use core::time::Duration;
Expand Down Expand Up @@ -150,7 +152,45 @@ impl Instant {
/// ```
#[stable(feature = "time2", since = "1.8.0")]
pub fn now() -> Instant {
Instant(time::Instant::now())
let os_now = time::Instant::now();

// And here we come upon a sad state of affairs. The whole point of
// `Instant` is that it's monotonically increasing. We've found in the
// wild, however, that it's not actually monotonically increasing for
// one reason or another. These appear to be OS and hardware level bugs,
// and there's not really a whole lot we can do about them. Here's a
// taste of what we've found:
//
// * #48514 - OpenBSD, x86_64
// * #49281 - linux arm64 and s390x
// * #51648 - windows, x86
// * #56560 - windows, x86_64, AWS
// * #56612 - windows, x86, vm (?)
// * #56940 - linux, arm64
// * https://bugzilla.mozilla.org/show_bug.cgi?id=1487778 - a similar
// Firefox bug
//
// It simply seems that this it just happens so that a lot in the wild
// we're seeing panics across various platforms where consecutive calls
// to `Instant::now`, such as via the `elapsed` function, are panicking
// as they're going backwards. Placed here is a last-ditch effort to try
// to fix things up. We keep a global "latest now" instance which is
// returned instead of what the OS says if the OS goes backwards.
//
// To hopefully mitigate the impact of this though a few platforms are
// whitelisted as "these at least haven't gone backwards yet".
if time::Instant::actually_monotonic() {
return Instant(os_now)
}

static LOCK: Mutex = Mutex::new();
static mut LAST_NOW: time::Instant = time::Instant::zero();
unsafe {
let _lock = LOCK.lock();
let now = cmp::max(LAST_NOW, os_now);
LAST_NOW = now;
Instant(now)
}
}

/// Returns the amount of time elapsed from another instant to this one.
Expand Down

2 comments on commit 255a3f3

@gabrieljones
Copy link

@gabrieljones gabrieljones commented on 255a3f3 Mar 2, 2020

Choose a reason for hiding this comment

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

This hammer is a code smell. This means that I can continue to get the wrong time even after a server's internal clock has been corrected by a time synchronizing service such as NTP. We should not be panicking when the diff between two instants is negative. That possibility should merely be stated in the API documentation for functions where it is likely for an inexperienced software engineer to make an assumption of monotonic increase on a system with more than one processor.

Time is an illusion. Lunchtime doubly so.

Don’t panic

https://gist.github.com/timvisee/fcda9bbdff88d45cc9061606b4b923ca

@wwylele
Copy link
Contributor

@wwylele wwylele commented on 255a3f3 Mar 2, 2020

Choose a reason for hiding this comment

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

@gabrieljones you want std::time::SystemTime for all the real-world time stuff and NTP stuff. std::time::Instance has no semantics on what second/minute/hour/day/month/year it is (so most of the gist you linked does not apply at all). And it is clearly documented that it is a monotonically nondecreasing clock, and the function elapsed/duration_since documented its panic behaviour. Let's not append more comments on this old commit.

Please sign in to comment.