tokio icon indicating copy to clipboard operation
tokio copied to clipboard

Allow disabling time auto-advance behavior in tests

Open l4l opened this issue 2 years ago • 4 comments

Basically a rebased version of @Dentosal patch from #5200 with slight naming changes.

Accepts a flag to explicitly disable auto-advancing behavior on paused time.

l4l avatar Oct 26 '23 17:10 l4l

cc @mcches

carllerche avatar Oct 27 '23 15:10 carllerche

Thanks for the PR. I read the discussion around the original issue, and I'm not sure I fully understand the motivation for this option.

Could you explain the use case a bit more? What is the problem caused by auto-advancing mocked time, and why not just disable time mocking entirely?

carllerche avatar Oct 27 '23 18:10 carllerche

Could you explain the use case a bit more? What is the problem caused by auto-advancing mocked time, and why not just disable time mocking entirely?

Although the behavior is quite reasonable this doesn't seem to be obvious to me. Otherwise I guess there won't be any notice in the pause fn. I thought as #4522 is still being open then the feature is still needed, though it seems #5115 actually solves the author's issue.

So apparently there's lack of sufficient motivation. For now I could think of two cases:

  1. The behavior is not obvious, one may want to disable it. It sill could be done with something like this though, but it's awkward and exploit internal (or just undocumented?) implementation of #5115:
use tokio::{task::spawn_blocking, time::sleep};

let _h = spawn_blocking(|| loop {}); // increments inhibit counter
sleep(Duration::from_secs(1)).await; // now it won't return
  1. Idle inhibit doesn't interoperate with other threads & timers, consider the following use-case:
use std::time::Duration;

use futures_intrusive::timer::{StdClock, Timer, TimerService};
use once_cell::sync::Lazy;

#[tokio::test]
async fn test_a() {
    tokio::time::pause();

    static CLOCK: Lazy<StdClock> = Lazy::new(|| StdClock::new());
    static TIME_SERVICE: Lazy<TimerService> = Lazy::new(|| TimerService::new(&*CLOCK));

    // Spawn thread that checks the timer for wake constantly.
    // Note that we do have synchronous control when to perform wakes here.
    std::thread::spawn(|| loop {
        TIME_SERVICE.check_expirations();
        std::thread::sleep(Duration::from_millis(10));
    });

    // Create first timer with shorter duration. This expected to resolve first.
    let timer1 = TIME_SERVICE.delay(Duration::from_millis(500));
    // And the second one from tokio.
    let timer2 = tokio::time::sleep(Duration::from_secs(1));

    // With disabled auto-advance, here we may adjust time as we want.

    tokio::select! {
        _ = timer1 => {},
        _ = timer2 => panic!("second timer goes earlier"),
    }
}

For me this tests always fails contrary to the code. So it's not about mocking itself but rather about expected amount of control.

I would consider including mock_time in the runtime-level configuration option, but I don't have a super strong opinion there.

Neither do I, but I guess explicit usage time::pause/auto_advance/start_paused implies mocking? Then we have something redundant like this:

time::pause() & co mock_time
called set mock_time explicit
called unset time::pause should panic?
uncalled set time::pause calls implicitly?
uncalled unset not mocked as expected

l4l avatar Oct 27 '23 23:10 l4l