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

time: Eliminate panics from Instant arithmetic #4461

Merged
merged 1 commit into from
Feb 6, 2022

Conversation

olix0r
Copy link
Member

@olix0r olix0r commented Feb 1, 2022

Motivation

Instant::duration_since, Instant::elapsed, and Instant::sub may
panic. This is especially dangerous when Instant::now travels back in
time. While this isn't supposed to happen, this behavior is highly
platform-dependent (e.g., rust-lang/rust#86470).

Solution

This change modifies the behavior of tokio::time::Instant to prevent
this class of panic, as proposed for std::time::Instant in
rust-lang/rust#89926.


@carllerche Suggested we may be able to make these changes to tokio::time::Instant to avoid std::time::Instant bugs. I've attempted to make similar changes in a variety of ecosystem projects (hyperium/hyper#2746, etc); but it seems higher-leverage to modify tokio to avoid this problem so that ecosystem projects can rely on Tokio's Instant type safely.

`Instant::duration_since`, `Instant::elapsed`, and `Instant::sub` may
panic. This is especially dangerous when `Instant::now` travels back in
time. While this isn't supposed to happen, this behavior is highly
platform-dependent (e.g., rust-lang/rust#86470).

This change modifies the behavior of `tokio::time::Instant` to prevent
this class of panic, as proposed for `std::time::Instant` in
rust-lang/rust#89926.
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

looks good to me!

Comment on lines -72 to -74
/// # Panics
///
/// This function will panic if `earlier` is later than `self`.
Copy link
Member

Choose a reason for hiding this comment

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

is it maybe worth explicitly noting somewhere that "unlike the std::time::Instant equivalent, this function does not panic" or similar? not a blocker.

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-time Module: tokio/time labels Feb 1, 2022
@olix0r olix0r requested a review from Darksonn February 3, 2022 17:12
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

This seems ok to me.

@Darksonn Darksonn merged commit fc4deaa into master Feb 6, 2022
@Darksonn Darksonn deleted the ver/panicless-instant branch February 6, 2022 15:20
ellenhp pushed a commit to killyourphone/tokio that referenced this pull request Feb 6, 2022
`Instant::duration_since`, `Instant::elapsed`, and `Instant::sub` may
panic. This is especially dangerous when `Instant::now` travels back in
time. While this isn't supposed to happen, this behavior is highly
platform-dependent (e.g., rust-lang/rust#86470).

This change modifies the behavior of `tokio::time::Instant` to prevent
this class of panic, as proposed for `std::time::Instant` in
rust-lang/rust#89926.
hawkw added a commit that referenced this pull request Feb 15, 2022
# 1.16.2 (February 15, 2022)

This release updates the minimum supported Rust version (MSRV) to 1.49,
the `mio` dependency to v0.8, and the (optional) `parking_lot`
dependency to v0.12. Additionally, it contains several bug fixes, as
well as internal refactoring and performance improvements.

### Fixed

- time: prevent panicking in `sleep` with large durations ([#4495])
- time: eliminate potential panics in `Instant` arithmetic on platforms
  where `Instant::now` is not monotonic ([#4461])
- io: fix `DuplexStream` not participating in cooperative yielding
  ([#4478])
- rt: fix potential double panic when dropping a `JoinHandle` ([#4430])

### Changed

- update minimum supported Rust version to 1.49 ([#4457])
- update `parking_lot` dependency to v0.12.0 ([#4459])
- update `mio` dependency to v0.8 ([#4449])
- rt: remove an unnecessary lock in the blocking pool ([#4436])
- rt: remove an unnecessary enum in the basic scheduler ([#4462])
- time: use bit manipulation instead of modulo to improve performance
  ([#4480])
- net: use `std::future::Ready` instead of our own `Ready` future
  ([#4271])
- replace deprecated `atomic::spin_loop_hint` with `hint::spin_loop`
  ([#4491])
- fix miri failures in intrusive linked lists ([#4397])

### Documented

- io: add an example for `tokio::process::ChildStdin` ([#4479])

### Unstable

The following changes only apply when building with `--cfg
tokio_unstable`:

- task: fix missing location information in `tracing` spans generated by
  `spawn_local` ([#4483])
- task: add `JoinSet` for managing sets of tasks ([#4335])
- metrics: fix compilation error on MIPS ([#4475])
- metrics: fix compilation error on arm32v7 ([#4453])

[#4495]: #4495
[#4461]: #4461
[#4478]: #4478
[#4430]: #4430
[#4457]: #4457
[#4459]: #4459
[#4449]: #4449
[#4462]: #4462
[#4436]: #4436
[#4480]: #4480
[#4271]: #4271
[#4491]: #4491
[#4397]: #4397
[#4479]: #4479
[#4483]: #4483
[#4335]: #4335
[#4475]: #4475
[#4453]: #4453
hawkw added a commit that referenced this pull request Feb 16, 2022
# 1.17.0 (February 16, 2022)

This release updates the minimum supported Rust version (MSRV) to 1.49,
the `mio` dependency to v0.8, and the (optional) `parking_lot`
dependency to v0.12. Additionally, it contains several bug fixes, as
well as internal refactoring and performance improvements.

### Fixed

- time: prevent panicking in `sleep` with large durations ([#4495])
- time: eliminate potential panics in `Instant` arithmetic on platforms
  where `Instant::now` is not monotonic ([#4461])
- io: fix `DuplexStream` not participating in cooperative yielding
  ([#4478])
- rt: fix potential double panic when dropping a `JoinHandle` ([#4430])

### Changed

- update minimum supported Rust version to 1.49 ([#4457])
- update `parking_lot` dependency to v0.12.0 ([#4459])
- update `mio` dependency to v0.8 ([#4449])
- rt: remove an unnecessary lock in the blocking pool ([#4436])
- rt: remove an unnecessary enum in the basic scheduler ([#4462])
- time: use bit manipulation instead of modulo to improve performance
  ([#4480])
- net: use `std::future::Ready` instead of our own `Ready` future
  ([#4271])
- replace deprecated `atomic::spin_loop_hint` with `hint::spin_loop`
  ([#4491])
- fix miri failures in intrusive linked lists ([#4397])

### Documented

- io: add an example for `tokio::process::ChildStdin` ([#4479])

### Unstable

The following changes only apply when building with `--cfg
tokio_unstable`:

- task: fix missing location information in `tracing` spans generated by
  `spawn_local` ([#4483])
- task: add `JoinSet` for managing sets of tasks ([#4335])
- metrics: fix compilation error on MIPS ([#4475])
- metrics: fix compilation error on arm32v7 ([#4453])

[#4495]: #4495
[#4461]: #4461
[#4478]: #4478
[#4430]: #4430
[#4457]: #4457
[#4459]: #4459
[#4449]: #4449
[#4462]: #4462
[#4436]: #4436
[#4480]: #4480
[#4271]: #4271
[#4491]: #4491
[#4397]: #4397
[#4479]: #4479
[#4483]: #4483
[#4335]: #4335
[#4475]: #4475
[#4453]: #4453
olix0r added a commit to olix0r/backoff that referenced this pull request Feb 23, 2022
`tokio::time::Instant` is a special `Instant` type that supports mocking
for tests (via [`tokio::time::pause`][pause]). Furthermore, it [avoids
panics][panic] in `Instant` arithmetic.

When using `tokio::time::Instant`, there's no need for a dependency on
the `instant` crate.

This change:

* makes the `instant` crate an optional dependency, enabled by default;
* uses `tokio::time::Instant` when the `tokio_1` feature is enabled and
  `instant` is not; and
* uses `std::time::Instant` when neither of these features are enabled.

The type is exposed publicly via `backoff::Instant` as a convenience.

This change is backwards-compatible and does not change the default
behavior.

[pause]: https://docs.rs/tokio/latest/tokio/time/fn.pause.html
[panic]: tokio-rs/tokio#4461

Signed-off-by: Oliver Gould <ver@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-time Module: tokio/time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants