Make test-util paused time fully deterministic

[sfackler]

The time driver stores an Instant internally used as a "base" for future
time calculations. Since this is generated as the Runtime is being
constructed, it previously always happened before the user had a chance
to pause time. The fractional-millisecond variations in the timing
around the runtime construction and time pause cause tests running
entirely in paused time to be very slightly deterministic, with the time
driver advancing time by 1 millisecond more or less depending on how the
sub-millisecond components of the Instants involved compared.

To avoid this, there is now a new option on runtime::Builder which
will create a Runtime with time "instantly" paused. This, along with a
small change to have the time driver use the provided clock as the
source for its start time allow totally deterministic tests with paused
time.

Closes #3179

[Darksonn]

Would probably be good with some sort of test.

[Darksonn]

So this makes it deterministic because pause is called before create_time_driver sets the start time?

[sfackler]

That, along with changing the ClockTime constructor to pull start_time out of the clock passed to it rather than going for the global clock (which isn't installed at that point so it falls back to the system clock).

[Darksonn]

Where is this panic?

[sfackler]

The call to clock.pause in Clock::new will panic when running on a multi threaded runtime.

[sfackler]

I added a stress test that asserts that timing around 10,000 random sleeps is consistent between runs. It's a bit gross, but seems like the best we can do probably.

[Darksonn]

Since making the test deterministic requires calling it on the builder, I agree that it should be added to the macro, but we should document it on both #[tokio::test] and tokio::time::pause.

[sfackler]

Done

[Darksonn]

The new attribute should be mentioned in the catch-all a few lines further down.

[sfackler]

Done

[Darksonn]

Currently this will fail to compile if used without the test-util feature, but making it detect that it the feature missing is somewhat cumbersome.

[sfackler]

Yeah - it looks like there's some detection for the multi-threaded runtime but it involves 2 macros and cfg'd imports in tokio which I don't think can easily scale to multiple features.

[sfackler]

I don't personally feel super strongly about supporting this in the macros, fwiw. I'd be fine leaving it out and adjusting the test to manually make the runtime.

[bdonlan]

Can we not just do e.g.:

if let Some(v) = config.start_paused {
  #[cfg(not(feature="test-util"))]
  {
    return Err(syn::Error::new_spanned(v.1, "start_paused requires feature test-util"));
  }
  rt = quote! { #rt.start_paused(#v) };
}

[Darksonn]

I have a memory with there being some sort of issue with forwarding the features to the tokio-macros crate (recall, it is a separate crate), but I'm not sure what it was.

[carllerche]

@sfackler what do you think about @bdonlan’s suggestion? @Darksonn wonders if it would work, it is worth a shot imo!

[sfackler]

Sure, I'll give it a try tonight. If it works, we could remove the main_rt and test_rt variants I think as well. Are those considered part of the crate's public API?

[Darksonn]

Let us just keep the others for now.

[sfackler]

Ah - the problem with that approach is that Cargo has no way currently to say "enable this feature on an optional dependency, but don't activate that dependency". So if we try to propagate features into tokio-macros, turning on any of those features in tokio automatically pulls in tokio-macros.

[carllerche]

LGTM thanks!

I added a note that we should try bdonlan’s suggestion re: a better compile time error when feature flags are missing, but it isn’t a blocker IMO.

I will let someone else merge to give an opportunity to disagree with my assessment.

[carllerche]

@sfackler what do you think about @bdonlan’s suggestion? @Darksonn wonders if it would work, it is worth a shot imo!