rust-lang / libs-team

The home of the library team
Apache License 2.0
115 stars 18 forks source link

Add a `thread::park_until` method #266

Closed neachdainn closed 11 months ago

neachdainn commented 12 months ago

Proposal

Problem statement

Having a standard way to park a thread for a set amount of time is a fantastic feature of the standard library. However, the thread::park_timeout function (and those that utilize it, such as mpsc::Receiver::recv_timeout) can cause unacceptable drift when being used in time-sensitive or soft real-time applications. Because thread::park_timeout has spurious wake-ups, there is risk of preemption happening between the calculation of the next Duration and the beginning of the park, which can lead to large inaccuracies.

Motivating examples or use cases

My specific use-case, for example, is robotic control loops where my code is at a high enough level where I don't need a full RTOS but I do want to make my control loops as accurate as possible. As part of this control loop, I would like to use code that is almost identical to futex::Parker::park_timeout except without the recalculation of timespec when there is a spurious wake-up.

Solution sketch

I am fairly unfamiliar with the parking implementations that aren't either the Linux or POSIX implementations, both of which can use absolute time and implement relative time on top of it. The solution for those platforms would be to modify the wait_timeout functions to either allow the caller to specify whether they want absolute or relative time, or to convert them to use absolute time and make the addition of now() be the responsibility of the caller.

As for other platforms, there may be ways to get the equivalent behavior but, at the very least, it could be approximated in the same way that park_timeout is currently approximating relative time.

Alternatives

It seems to me that there is really only one reasonable alternative, and that is to do what my code currently does: use libc directly to accomplish this in a platform-specific way. From my perspective, the major disadvantages of this option is that it means that all methods which use thread::park_timeout will suffer the same problem without the option to convert them to using this hypothetical thread::park_until. There is also the issue of individuals having to either write this themselves or use an external dependency but that seems relatively minor.

Any other alternatives seem like they would end up requiring the standard library to take a stronger position on the accuracy of thread::park_timeout, which is a fairly complicated topic and probably not the job of the standard library.

Links and related work

Michael Kerrisk discusses this some in "The Linux Programming Interace" §23.5.4 (Improved High-Resolution Sleeping: _clocknanosleep()). I am willing to find and link additional related work if that would be deemed valuable.

neachdainn commented 12 months ago

Please note that I am more than willing to make these changes myself but I only have the knowledge on how to do this for the futex and pthreads implementations. Other implementations would either require me to do some research or would just be an approximation based on the code that already exists for thread::park_timeout.

joboet commented 11 months ago

Please note that I am more than willing to make these changes myself but I only have the knowledge on how to do this for the futex and pthreads implementations. Other implementations would either require me to do some research or would just be an approximation based on the code that already exists for thread::park_timeout.

I wrote most of the thread parking implementations currently in use, so if you have any questions, feel free to ping me.

Amanieu commented 11 months ago

We discussed this in the libs-api meeting yesterday: we're happy to accept this, but care should be taken to ensure that the sleep here actually corresponds to the clock that Instant uses. Specifically with regards to issues like https://github.com/rust-lang/rust/issues/79462.

neachdainn commented 11 months ago

We discussed this in the libs-api meeting yesterday: we're happy to accept this, but care should be taken to ensure that the sleep here actually corresponds to the clock that Instant uses. Specifically with regards to issues like rust-lang/rust#79462.

I will add this to TODO list, then! That being said, I'll reiterate that I'm only familiar with the futex and pthreads implementations, so someone with knowledge of the other platforms should closely scrutinize my work (or just implement it themselves).

ChrisDenton commented 11 months ago

sleep here actually corresponds to the clock that Instant uses

I'm not sure this is going to be entirely possible. The sleep will be whatever the system's futex (or equivalent) gives you. Which may or may not be consistent with the clock used for Instant.

neachdainn commented 11 months ago

I'm not sure this is going to be entirely possible. The sleep will be whatever the system's futex (or equivalent) gives you. Which may or may not be consistent with the clock used for Instant.

In the current implementation, Instant uses CLOCK_MONOTONIC and the futex operations are FUTEX_WAIT_BITSET and FUTEX_WAKE, both of which use CLOCK_MONOTONIC (unless FUTEX_CLOCK_REALTIME is specified). So it should be trivially possible unless the clock behind Instant changes.

That being said, FUTEX_CLOCK_REALTIME was added in Linux 2.6.28, so I'll need to double check that either the minimum supported version is later than that or what the behavior is before that flag.

neachdainn commented 10 months ago

I actually should have checked around a little more: this is very relevant and the currently implemented APIs (mpsc::Receiver::recv_deadline) currently have the same "bug" I describe here.

...but I also would say that if you need that level of fidelity, using an MPSC as the timeout is probably too high-level to be appropriate while thread::park_until is primitive enough that I think it matters more.