Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a thread::park_until method #266

Closed
neachdainn opened this issue Sep 8, 2023 · 7 comments
Closed

Add a thread::park_until method #266

neachdainn opened this issue Sep 8, 2023 · 7 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@neachdainn
Copy link

neachdainn commented Sep 8, 2023

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: clock_nanosleep()). I am willing to find and link additional related work if that would be deemed valuable.

@neachdainn neachdainn added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Sep 8, 2023
@neachdainn
Copy link
Author

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
Copy link
Member

joboet commented Oct 1, 2023

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
Copy link
Member

Amanieu commented Oct 4, 2023

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.

@Amanieu Amanieu closed this as completed Oct 4, 2023
@Amanieu Amanieu added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label Oct 4, 2023
@neachdainn
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Author

neachdainn commented Oct 24, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

4 participants