The `thread::sleep_until` test frequently fails under Windows 7

[PaulDance]

As part of our (lately subpar) maintenance of the Windows 7 target, we run the library's tests. Since 1.90 while building for stable, we have noticed that the library/std/tests/thread.rs::sleep_until test started to fail quite frequently.

The test itself is quite simple:

rust/library/std/tests/thread.rs

Lines 23 to 31 in fa5eda1

	fn sleep_until() {
	let now = Instant::now();
	let period = Duration::from_millis(100);
	let deadline = now + period;
	thread::sleep_until(deadline);
	let elapsed = now.elapsed();
	assert!(elapsed >= period);
	}

and fails on assertion misses such as:

running 1 test
[.../lib.rs:125:9] elapsed = 95.9499ms
[.../lib.rs:125:9] period = 100ms

thread 'tests::sleep_until' panicked at [...]/lib.rs:126:9:
assertion failed: elapsed >= period
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test tests::sleep_until ... FAILED

These errors are appropriate, but there is still something not quite right somewhere.

The current implementation of sleep_until under Windows is still a proxy to sleep(deadline - Instant::now()) if I'm not mistaken, so the issue could come from sleep itself. In fact, the issue can also be reproduced with the slightly simpler:

#[test]
fn sleep() {
    let now = Instant::now();
    let period = Duration::from_millis(100);
    thread::sleep(period);

    let elapsed = now.elapsed();
    dbg!(elapsed, period);
    assert!(elapsed >= period);
}

Under Windows 7, the high precision timer API is not available, so the standard Sleep function gets used with the given Duration's milliseconds. On the one hand, Rust's thread::sleep documents that "It will never sleep less [than the duration specified].", while on the other hand Window's Sleep only documents that "The system clock 'ticks' at a constant rate. If dwMilliseconds is less than the resolution of the system clock, the thread may sleep for less than the specified length of time. If dwMilliseconds is greater than one tick but less than two, the wait can be anywhere between one and two ticks, and so on.", but not more specifically that it assuringly cannot suspend for less than the specified length of time. Trying to get or set the "accuracy of the sleep interval" using the recommended methods for example to be 1ms did not help. Could it be that Windows 7's implementation of the function is just not that precise anyway?

The test actually has two facets: sleeping and measuring time. The latter could also be the cause of the errors here. As documented and implemented, the Instant::now bases itself on QueryPerformanceCounter under Windows. The function itself does not document much, but "Acquiring high-resolution time stamps" gives some lower-level details. Among these, it does mention a high adherence to hardware features. Could it therefore be the cause here? We use QEMU for running Windows 7 in VMs. I have tried to enable as much hardware-related options of QEMU to see if it could fix the problem, but it was to no avail. If it is indeed the cause, it is more a Windows 7 or QEMU issue?

We are having trouble answering these questions ourselves. Does anyone know what could be happening here? In particular, is it something that will never realistically work under Windows 7 or the testing facilities we use, while under actual hardware there would be no issue, and so the test should be simply ignored?

Note that I can reproduce these errors under our 1.89 build already, so considering the test landed in stable under 1.90, the issue might not be recent. Also, although it might be obvious, I cannot reproduce this under Windows 10+. This could only be a testing issue, so I'm not tagging this as a bug just yet.

cc @roblabla

@rustbot label F-sleep_until O-windows-7 T-libs I-flaky-test A-time A-thread E-help-wanted

[ChrisDenton]

I do think this is very likely to be a problem with Sleep on Windows 7 and not just an artefact of your testing environment.

Though both of the workarounds I can think may lead to waiting a fair bit longer than the specified time. But that's always a risk when the timer resolution is 15.625 ms by default.

• use Sleep in a loop until at least the duration has passed
• add some constant to non-zero durations passed to sleep. Adding 7.8125ms (i.e. half the default timer resolution) may be sufficient to cause it to wake at the later time. Ideally the value would adjust to the current timer resolution though.

[Fulgen301]

If the sleep implementation is modified to always execute Sleep instead of using a high-precision timer, the test also fails on Windows 11.

Though both of the workarounds I can think may lead to waiting a fair bit longer than the specified time.

It should be fine if we round up the duration to the next multiple of the timer frequency:

If dwMilliseconds is greater than one tick but less than two, the wait can be anywhere between one and two ticks, and so on.

[ChrisDenton]

Hm, I've not personally been able to trigger it on my local machine but that might have something to do with my setup:

test program

use std::time::{Duration, Instant};
fn main() {
    for _ in 0..u32::MAX {
        let now = Instant::now();
        let period = Duration::from_millis(100);
        Sleep(dur2timeout(period));
        let elapsed = now.elapsed();
        assert!(elapsed >= period, "{elapsed:?} < {period:?}");
    }
}

pub fn dur2timeout(dur: Duration) -> u32 {
    dur.as_secs()
        .checked_mul(1000)
        .and_then(|ms| ms.checked_add((dur.subsec_nanos() as u64) / 1_000_000))
        .and_then(|ms| ms.checked_add(if dur.subsec_nanos() % 1_000_000 > 0 { 1 } else { 0 }))
        .map(|ms| if ms > <u32>::MAX as u64 { INFINITE } else { ms as u32 })
        .unwrap_or(INFINITE)
}

const INFINITE: u32 = u32::MAX;

unsafe extern "system" {
    safe fn Sleep(dwmilliseconds: u32);
}

[Fulgen301]

I must have borked testing initially, because after several tests with the standard library test and your test program I have not been able to get it to fail once, though I have been able to reproduce the failure occasionally within a Windows 7 VM. (Amusingly, I didn't see the problem in Windows 8.1 and Windows 10 VMs.)

Ideally the value would adjust to the current timer resolution though.

This has the problem of determining the current timer resolution; Windows 10 2004 switched from global state to process-local state, with a bunch of new rules for processes that don't request a timer resolution. timeGetDevCaps does not expose the current value, and NtQueryTimerResolution does not have documentation on how it interacts with those changes at all, as it doesn't have documentation in the first place.

C++ gets away with only calling Sleep by allowing implementations some wiggle room with sleeping shorter or longer than requested.

[PaulDance]

Thank you both for your feedback.

I do think this is very likely to be a problem with Sleep on Windows 7 and not just an artifact of your testing environment.

OK, that's a form of relief for me! Should it therefore be considered an actual bug?

add some constant to non-zero durations passed to sleep

That seems like the better option to reach the desired guarantees in my opinion. The other one feels more hackish to me as it would require basically invoking Instant::now and elapsed in the middle of sleep in order to reach pretty much the same result I think.

NtQueryTimerResolution does not have documentation on how it interacts with those changes at all, as it doesn't have documentation in the first place.

True, but some original research indicates that it does, so we could get away with that? At the very least, experimenting with it should be easy, because it would only be matter of calling NtQueryTimerResolution once at the beginning, then timeBeginPeriod(1), and then NtQueryTimerResolution again to try observing a change. I'll try that on Monday.

I'll also experiment with raw Sleep(100) calls in order to first reproduce the issue at a lower level and then trying to see if adding the current timer period or its half would work in fixing the issue.

[ChrisDenton]

Should it therefore be considered an actual bug?

I think so, yes. It's either a documentation bug or an implementation bug. Though confirming it using raw API calls would indeed be helpful.

We could consider this a doc bug and weaken the documentation of thread::sleep or add a platform specific caveat for Windows 7. But I suspect libs-api may not want to do that if Windows 7 is the only affected platform.

Alternatively we consider it an implementation bug and the mitigations discussed above would be more appropriate. But I'm having second thoughts about NtQueryTimerResolution. The timer resolution could technically change just before the call to Sleep. So calling NtQueryTimerResolution to get the current resolution wouldn't necessarily guarantee anything.