hyper-util: regression in paused time compatibility

[asmello]

Version
hyper version 1.7.0 (not relevant AFAIK), hyper-util version 0.1.16

Platform
MacOS

Description

Now, I realise nowhere (that I know) Hyper promises to be compatible with Tokio's paused-time feature. But neither does it document (that I know) that it isn't.

The issue I'm reporting is fundamentally due to an assumption that hyper-util makes that std::time::Instant always matches tokio::time::Instant. This is not true when paused time is enabled.

This probably spans multiple places, but let's focus on the one I've investigated: the IdleTask implementation.

1. Initially std::time::Instant and tokio::time::Instant start the same.
2. When IdleTask is first polled, its internal tokio::time::Sleep is configured to trigger after std::time::Instant::now() plus the timeout interval. Since the clocks are still in sync (assuming it gets polled immediately, which seems to be the case empirically), this does the correct thing and the deadline is set 90s in the future (assuming the default timeout is used) according to both clocks.
3. Tokio will let other tasks do work, and once an idle state is reached, it will auto-advance time until the next scheduled event. Let's suppose this is the deadline set by IdleTask.
4. Now over 90s have passed in Tokio's clock, but very little real time has actually passed. The clocks have drifted. When we configure the next deadline here, it will be based off actual time. The actual time plus 90s is going to be very close to Tokio's perceived present, maybe even in the past. This causes the task to be immediately awaken and the future polled again.
5. In the next iteration, very little actual time will have passed, so the next deadline is still approximately 90s past the starting time. Again this is in the present/past from Tokio's perspective, so again it schedules the future to be polled immediately.
6. This creates a busy loop. The program hangs.

Interestingly, paused time used to almost work up to release 0.1.15. This is because the next deadline was calculated relative to the previous one, rather than the realtime clock. The drift check in L801 would never trigger, because it used real time, which after the first deadline was always set in the past from Tokio's perspective. Since the two clocks started in sync, and Tokio was nice enough to wake up the task at the right times, the previous deadline + timeout interval were approximately equal to tokio::time::Instant::now() at each iteration, so the deadlines were set more or less correctly anyway.

I think even prior to 0.1.16 enough drift between computed deadlines (always spaced exactly this.duration apart) and Tokio's simulated time would accumulate that you'd eventually hit the drift check at L801, and then it would hit a busy loop all the same. But that was relatively unlikely, since paused time is mostly used for testing, and tests tend to not run over that long (even in simulated time).

Suggested fix

I believe the right fix for this is to not transparently convert between std::time and tokio::time types like here and here. The same problem would likely occur in other runtimes if they diverged at all from std::times monotonic clock implementation.

Instead, hyper-util should probably use an abstraction like what it does for the Sleep methods, where it defers to a boxed trait object for the actual implementation. So instead of directly calling std::time::Instant::now(), it should call some TimeProvider::now() method that can be configured to one runtime or another.

Alternatively, the logic for determining and setting the next deadline could be moved to the runtime-specific implementations, so that IdleTask would never have to directly manipulate Instants.

I think this part of the codebase is currently being revised, so maybe this is not worth fixing. But I figured I'd at least report the perceived regression — I've observed tests that were working just fine break after the 0.1.16 patch release. Even if paused time compatibility is not part of hyper-utils promises, this isn't ideal.

[asmello]

BTW happy to contribute a PR to revert to the previous behaviour, if that seems more reasonable. I can also attempt implementing one of the suggested fixes if given the greenlight.

[seanmonstar]

I assume it was this change that made the difference: hyperium/hyper-util#216

There was another recent request to allow supplying a clock, though for different reasons: #3943. As I mentioned in there, I wouldn't be against it in principle, but it'd need to be done in a way that doesn't break backwards compatibility. The Timer trait is in hyper, and I don't want to do v2 (any time soon).

[asmello]

I assume it was this change that made the difference: hyperium/hyper-util#216

Yes, indeed!

I think the change proposed in #3943 would solve this problem alright. I'll have a look later to see if it can be done in a backwards compatible way. It would probably be a bit hackish.

Regardless, I think it would make sense to update docs (both here and in Tokio). The tokio::time::Instant::from_std docs don't warn about this potential pitfall. And naively one might assume that any library using Tokio would be paused-time compatible.

Would you take a PR to add a note somewhere? For visibility the top-level module of hyper would be ideal, but I'm open to adding it in a more specific place (like here).

[seanmonstar]

The hyper crate does not directly depend on Tokio, at least publicly, so I don't think it belongs there. The hyper::rt::Timer is oblivious to what the implementation is, and whatever testing utilities that implementation might have.

However, is the issue that we explicitly grab Instant::now() again, instead of just adding duration to the previous one? I'd think that the implementation of hyper_util::rt::TokioTimer could try to make it work.

[dswij]

Paused time in tokio is for testing purposes only. tokio::time::Instant itself is a wrapper on top of std::time::Instant. I suppose this would be more of an incompatibility with Tokio's testing features than a bug.

I'd think that the implementation of hyper_util::rt::TokioTimer could try to make it work.

Or we can use timer.sleep(..) instead of timer.sleep_until(..) in hyper-util. It should be equivalent, but it will work with the tokio test features.

Another one is probably implementing a test hyper_util::rt::TokioTimer that immediately subtracts Instant::now() and calls self.sleep. But this is too janky IMO, since the second call to now() results in a different value.

This does give a point for #3943 to be worked on

[xetqL]

Hi, I have updated #3943 with a proposal to make it backward compatible.

If fine with y'all I can work on an implementation.