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

Support tokio::time::Instant in SystemClock #53

Closed
wants to merge 1 commit into from

Conversation

olix0r
Copy link

@olix0r olix0r commented Feb 23, 2022

tokio::time::Instant is a special Instant type that supports mocking
for tests (via tokio::time::pause). Furthermore, it avoids
panics
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 alter the default
behavior.

`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>
@nightkr
Copy link
Contributor

nightkr commented Feb 23, 2022

I think it's pretty confusing to start mixing type aliases around like this based on what features are enabled. This PR would make it so that backoff::Instant == std::time::Instant unless the feature set matches tokio && !instant.

I'd rather pick one of:

  1. Make backoff::Instant a newtype that implements the appropriate From instances for whatever features are enabled
  2. Introduce a new TokioClock type users would use instead of SystemClock

@olix0r
Copy link
Author

olix0r commented Feb 23, 2022

I think it's pretty confusing to start mixing type aliases around like this based on what features are enabled. This PR would make it so that backoff::Instant == std::time::Instant unless the feature set matches tokio && !instant.

Yeah, I don't disagree; though I'll note that the instant crate is already playing these games (albeit depending on platform, instead of feature flag).

  1. Make backoff::Instant a newtype that implements the appropriate From instances for whatever features are enabled
  2. Introduce a new TokioClock type users would use instead of SystemClock

In both of these cases, I think all of the Instants would have to be converted to std::time::Instant -- which is OK, but we'd need to audit the code for uses of elapsed/duration_since/sub (which all can panic) to make sure we're properly guarding against cases that can panic.

@olix0r
Copy link
Author

olix0r commented May 9, 2022

This seems less relevant now that Rust 1.60 has removed panics from Duration operations

@olix0r olix0r closed this May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants