util: Ensure UninterruptibleSleep does not return too early #32199

1. 
   maflcko commented at 9:35 am on April 2, 2025: member

   Both, sleep_for and sleep_until may sleep less than the specified time, because the implementation is free to be susceptible to system clock adjustments (albeit it is recommended to not be).

   Clock adjustments should be rare, and I don’t think any caller of UninterruptibleSleep (except for one unit test) relies on it always sleeping at least that much.

   However, it shouldn’t hurt to fix this by ensuring a steady clock is used to measure the lower bound.

   Presumably fixes https://github.com/bitcoin/bitcoin/issues/32197

2. util: Ensure UninterruptibleSleep does not return too early faa0e87e0b
3. 
   DrahtBot commented at 9:35 am on April 2, 2025: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32199.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	w0xlt

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #32061 (Replace libevent with our own HTTP and socket-handling implementation by pinheadmz)
   • #31672 (rpc: add cpu_load to getpeerinfo by vasild)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

4. DrahtBot added the label Utils/log/libs on Apr 2, 2025
5. 
   laanwj commented at 11:18 am on April 2, 2025: member

   Both, sleep_for and sleep_until may sleep less than the specified time, because the implementation is free to be susceptible to system clock adjustments (albeit it is recommended to not be).

   Huh. You would expect sleep_for to map directly to OS delay functions. This implementation involving a loop and checking the time seems over-compicated to me. But if it fixes an actual issue that’s fine.

6. 
   maflcko commented at 12:52 pm on April 2, 2025: member

   From https://en.cppreference.com/w/cpp/thread/sleep_for I got:

   If an implementation uses a system clock instead, the wait time may also be sensitive to clock adjustments.

   and similarly for https://en.cppreference.com/w/cpp/thread/sleep_until.

   (It is correctly implemented in Rust, but that is probably unrelated and out of scope here: https://doc.rust-lang.org/std/thread/fn.sleep.html says “It will never sleep less.”)

7. 
   w0xlt commented at 9:36 pm on April 2, 2025: contributor

   Code review ACK https://github.com/bitcoin/bitcoin/pull/32199/commits/faa0e87e0be8289307829f77b1c9e9854c175c36

   This basically addresses the issue by continuously checking the time and sleeping until the target time is reached, making it effectively “uninterruptible.” (not susceptible to interruptions or adjustments to the Windows clock).

8. 
   maflcko commented at 9:22 am on April 3, 2025: member

   But if it fixes an actual issue that’s fine.

   I don’t have Windows, so I can’t test if this actually fixes the issue. And on a second look, it looks like GCC is just passing this on to Sleep: https://github.com/gcc-mirror/gcc/blame/92ca72b41a74aef53978cadbda33dd38b69d3ed3/libstdc%2B%2B-v3/src/c%2B%2B11/thread.cc#L275-L280, which would be https://learn.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-sleep, but that page isn’t mentioning anything about time adjustments.

   So just from reading the docs, it would appear that this may not fix the issue, depending on what the issue is.

   ( Unrelatedly, GCC maps sleep_until to sleep_for (https://github.com/gcc-mirror/gcc/blob/1bfc026035e6bade36538077dc1dd4f9baff0bf2/libstdc%2B%2B-v3/include/bits/this_thread_sleep.h#L89-L109) and sleep_for, on GCC is doing a while-loop on non-Windows as well, just like the code in this pull right now.

   Going further, libc++ just maps the sleep_until (at least the steady one) to sleep_for (https://github.com/llvm/llvm-project/blob/edc22c64e527171041876f26a491bb1d03d905d5/libcxx/include/__thread/this_thread.h#L66), which again is mapped directly to thrd_sleep (https://github.com/llvm/llvm-project/blob/3295970d846b0d820b863f9eeac559b80239297e/libcxx/include/__thread/support/c11.h#L169C3-L169C13) or Sleep (https://github.com/llvm/llvm-project/blame/3295970d846b0d820b863f9eeac559b80239297e/libcxx/src/support/win32/thread_win32.cpp#L194), or nanosleep (historically), and the only one with a loop: https://github.com/llvm/llvm-project/blob/3295970d846b0d820b863f9eeac559b80239297e/libcxx/include/__cxx03/__thread/support/pthread.h#L198

   Neither https://en.cppreference.com/w/c/thread/thrd_sleep, nor https://www.man7.org/linux/man-pages/man2/nanosleep.2.html mention anything about time adjustments, so :shrug: . )

   If it is important to fix the intermittent CI failure in one go, the only reliable way would be to remove the test.

9. 
   laanwj commented at 10:46 am on April 7, 2025: member

   Neither https://en.cppreference.com/w/c/thread/thrd_sleep, nor https://www.man7.org/linux/man-pages/man2/nanosleep.2.html mention anything about time adjustments, so 🤷 .

   Right. OS sleep functions generally just pause the thread in the scheduler for a given number of timeslices. Making them dependent on system time would break a lot of assumptions.

   If it is important to fix the intermittent CI failure in one go, the only reliable way would be to remove the test.

   Seems the best option to me. Anything realtime-related tends to make tests more brittle, and we don’t need to test all kinds of base OS or C++ library functionality as part of the bitcoin core tests. Especially as none of our uses of sleep are that critical.

10. 
    maflcko commented at 10:56 am on April 7, 2025: member

    If it is important to fix the intermittent CI failure in one go, the only reliable way would be to remove the test.

    Seems the best option to me. Anything realtime-related tends to make tests more brittle, and we don’t need to test all kinds of base OS or C++ library functionality as part of the bitcoin core tests. Especially as none of our uses of sleep are that critical.

    Yeah, I agree.

11. maflcko closed this on Apr 7, 2025

    ---

12. maflcko deleted the branch on Apr 7, 2025
13. 
    Jassor909 commented at 3:44 pm on April 7, 2025: none

    .

Contributors
maflcko DrahtBot laanwj w0xlt Jassor909

Labels
Utils/log/libs