kube icon indicating copy to clipboard operation
kube copied to clipboard

Use `tokio::time::Instant` as a time source

Open olix0r opened this issue 3 years ago • 3 comments

Would you like to work on this feature?

maybe

What problem are you trying to solve?

tokio::time::Instant has two important benefits over std::time::Instant (as well as the instant crate):

  1. It supports mocking for tests via tokio::time::pause, etc. This allows a test to control how time advances, interacting well with the tokio timers.
  2. It avoids panics. Especially on Amazon Linux/EKS, we have reports of time arithmetic causing runtime panics due to bugginess in the OS's time source and Rust's standard library.

Describe the solution you'd like

We should replace uses of std::time::Instant with tokio::time::Instant. We may also want to add a clippy configuration including:

disallowed-types = [
    # Use parking_lot instead.
    "std::sync::Mutex",
    "std::sync::RwLock",

    # Use tokio::time::Instant instead.
    "std::time::Instant",
]

See also:

  • https://github.com/ihrwein/backoff/pull/53

Describe alternatives you've considered

Don't allow time mocking, wait for Rust std lib to solve panics in Instant.

Documentation, Adoption, Migration Strategy

No response

Target crate for feature

kube-runtime

olix0r avatar Feb 23 '22 15:02 olix0r

that sounds great to me! if you have time, then happy to take a pr on this.

clux avatar Feb 23 '22 15:02 clux

We already use tokio::time::Instant in most places because of the mocking, and the case of backoff_reset_timer looks like an oversight. :+1: from me.

nightkr avatar Feb 23 '22 15:02 nightkr

@clux I think it's a pretty trivial PR but, practically, it's probably blocked on ihrwein/backoff#53 -- while we can change the backoff clock, we can't really avoid panics due backoff's calls to duration_since, etc. I'll follow-up here when that PR shakes out a bit.

olix0r avatar Feb 23 '22 15:02 olix0r