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

Use tokio::time::Instant as a time source #840

Open
olix0r opened this issue Feb 23, 2022 · 3 comments
Open

Use tokio::time::Instant as a time source #840

olix0r opened this issue Feb 23, 2022 · 3 comments
Labels
help wanted Not immediately prioritised, please help! runtime controller runtime related

Comments

@olix0r
Copy link
Contributor

olix0r commented Feb 23, 2022

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:

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

@clux clux added the runtime controller runtime related label Feb 23, 2022
@clux
Copy link
Member

clux commented Feb 23, 2022

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

@clux clux added the help wanted Not immediately prioritised, please help! label Feb 23, 2022
@nightkr
Copy link
Member

nightkr commented Feb 23, 2022

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

@olix0r
Copy link
Contributor Author

olix0r commented Feb 23, 2022

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Not immediately prioritised, please help! runtime controller runtime related
Projects
None yet
Development

No branches or pull requests

3 participants