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: advance frozen time in park_timeout #2059

Merged
merged 1 commit into from
Jan 6, 2020
Merged

Conversation

carllerche
Copy link
Member

This patch improves the behavior of frozen time (a testing utility made
available with the test-util feature flag). Instead of of requiring
time::advance to be called in order to advance the value returned by
Instant::now, calls to time::Driver::park_timeout will use the
provided duration to advance the time.

This is the desired behavior as the timeout is used to indicate when the
next scheduled delay needs to be fired.

This patch improves the behavior of frozen time (a testing utility made
available with the `test-util` feature flag). Instead of of requiring
`time::advance` to be called in order to advance the value returned by
`Instant::now`, calls to `time::Driver::park_timeout` will use the
provided duration to advance the time.

This is the desired behavior as the timeout is used to indicate when the
next scheduled delay needs to be fired.
Copy link
Member

@gardnervickers gardnervickers left a comment

Choose a reason for hiding this comment

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

LGTM

@carllerche carllerche merged commit f000600 into master Jan 6, 2020
@carllerche carllerche deleted the tweak-frozen-time branch January 10, 2020 19:29
Marwes pushed a commit to Marwes/tokio that referenced this pull request Jul 7, 2020
This was changed in tokio-rs#2059. This had me extremely confused for some time
as my timeouts fired immediately, without the wrapped future that were
waiting on IO to actually run long enough.

I am not sure about the exact wording here but this had me very confused
for some time. Deprecating "pause" and giving it a more accurate name
may be a good idea as well.

```rust
async fn timeout_advances() {
    time::pause();

    timeout(ms(1), async {
        // Change to 1 and the this future resolve, 2 or
        // more and the timeout resolves
        for _ in 0..2 {
            tokio::task::yield_now().await
        }
    })
    .await
    .unwrap();
}

```
Marwes pushed a commit to Marwes/tokio that referenced this pull request Jul 7, 2020
This was changed in tokio-rs#2059. This had me extremely confused for some time
as my timeouts fired immediately, without the wrapped future that were
waiting on IO to actually run long enough.

I am not sure about the exact wording here but this had me very confused
for some time. Deprecating "pause" and giving it a more accurate name
may be a good idea as well.

```rust
async fn timeout_advances() {
    time::pause();

    timeout(ms(1), async {
        // Change to 1 and the this future resolve, 2 or
        // more and the timeout resolves
        for _ in 0..2 {
            tokio::task::yield_now().await
        }
    })
    .await
    .unwrap();
}

```
hawkw pushed a commit that referenced this pull request Jul 10, 2020
…#2647)

* doc: Update the docs of "pause" to state that time will still advance

This was changed in #2059. This had me extremely confused for some time
as my timeouts fired immediately, without the wrapped future that were
waiting on IO to actually run long enough.

I am not sure about the exact wording here but this had me very confused
for some time. Deprecating "pause" and giving it a more accurate name
may be a good idea as well.

```rust
async fn timeout_advances() {
    time::pause();

    timeout(ms(1), async {
        // Change to 1 and the this future resolve, 2 or
        // more and the timeout resolves
        for _ in 0..2 {
            tokio::task::yield_now().await
        }
    })
    .await
    .unwrap();
}

```

* Update tokio/src/time/clock.rs

Co-authored-by: Alice Ryhl <alice@ryhl.io>

Co-authored-by: Alice Ryhl <alice@ryhl.io>
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.

None yet

3 participants