Skip to content

multiple platforms incorrectly impl thread::sleep with Duration::as_micros #129212

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

Closed
workingjubilee opened this issue Aug 17, 2024 · 4 comments · Fixed by #129588
Closed

multiple platforms incorrectly impl thread::sleep with Duration::as_micros #129212

workingjubilee opened this issue Aug 17, 2024 · 4 comments · Fixed by #129588
Labels
A-thread Area: `std::thread` C-bug Category: This is a bug. O-ESP-IDF Target: Espressif IoT Development Framework O-hermit Operating System: Hermit T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@workingjubilee
Copy link
Member

workingjubilee commented Aug 17, 2024

Multiple targets currently use Duration::as_micros to implement conversion from a Duration to a value in microseconds. However, thread::sleep currently reads as so:

Platforms which do not support nanosecond precision for sleeping will have dur rounded up to the nearest granularity of time they can sleep for.

Up. Note this Playground which means Duration::as_micros is not a valid implementation of this spec.

Note that this is not the only problem that espidf poses with respect to its std implementation: #129136

Meta

rustc --version --verbose:

rustc 1.82.0-nightly (2c93fabd9 2024-08-15)
binary: rustc
commit-hash: 2c93fabd98d2c183bcb3afed1f7d51b2517ac5ed
commit-date: 2024-08-15
host: x86_64-unknown-linux-gnu
release: 1.82.0-nightly
LLVM version: 19.1.0
@workingjubilee workingjubilee added C-bug Category: This is a bug. O-ESP-IDF Target: Espressif IoT Development Framework labels Aug 17, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 17, 2024
@workingjubilee
Copy link
Member Author

workingjubilee commented Aug 17, 2024

First found on espidf:

#[cfg(target_os = "espidf")]
pub fn sleep(dur: Duration) {
let mut micros = dur.as_micros();
unsafe {
while micros > 0 {

cc @ivmarkov @MabezDev @SergioGasquez

@workingjubilee workingjubilee added the O-hermit Operating System: Hermit label Aug 17, 2024
@workingjubilee workingjubilee changed the title espidf platforms don't impl thread::sleep correctly re: Duration multiple platforms incorrectly impl thread::sleep with Duration::as_micros Aug 17, 2024
@workingjubilee
Copy link
Member Author

Same bug:

#[inline]
pub fn sleep(dur: Duration) {
unsafe {
hermit_abi::usleep(dur.as_micros() as u64);
}
}

cc @stlankes @mkroening

@workingjubilee workingjubilee added T-libs Relevant to the library team, which will review and decide on the PR/issue. A-thread Area: `std::thread` and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 17, 2024
@ivmarkov
Copy link
Contributor

ivmarkov commented Aug 18, 2024

First found on espidf:

#[cfg(target_os = "espidf")]
pub fn sleep(dur: Duration) {
let mut micros = dur.as_micros();
unsafe {
while micros > 0 {

cc @ivmarkov @MabezDev @SergioGasquez

@workingjubilee

Agreed - problem is clear and I'll fix shortly - thanks for noticing!

An interesting question here is whether it is allowed - in thread:sleep - to busy-wait for sleeping? Because this is exactly what would happen on the ESP IDF if the supplied duration is less than 10ms (milliseconds) - i.e. the FreeRTOS sys-tick. Given that the thread:sleep documentation is silent in this regard, I suggest to keep the current behavior (i.e. busy-waiting for durations < 10ms) as that allows callers of thread::sleep to "at least do some sleep - busy-looping or not".

Note that this is not the only problem that espidf poses with respect to its std implementation: #129136

I don't think this is a problem on the ESP IDF, as it does not support POSIX signals in the first place. Or processes thereof.

@workingjubilee
Copy link
Member Author

workingjubilee commented Aug 18, 2024

Yes, I believe busy-waiting is a valid implementation of sleep, and especially in particular, if we were to have a hypothetical fn sleep_can_sleep_for_this_long(dur: Duration) -> bool that returned false if the platform didn't have an actual syscall for sleeping for that long, I would expect busy-waiting to be on the other side of the if/else almost every single time.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 21, 2024
Fix `thread::sleep` Duration-handling for ESP-IDF

Addresses the ESP-IDF specific aspect of rust-lang#129212

#### A short summary of the problems addressed by this PR:
================================================

1. **Problem 1** - the current implementation of `std::thread::sleep` does not properly round up the passed `Duration`

As per the documentation of `std::thread::sleep`, the implementation should sleep _at least_ for the provided duration, but not less. Since the minimum supported resolution of the `usleep` syscall which is used with ESP-IDF is one microsecond, this means that we need to round-up any sub-microsecond nanos to one microsecond. Moreover, in the edge case where the user had passed a duration of < 1000 nanos (i.e. less than one microsecond), the current implementation will _not_ sleep _at all_.

This is addressed by this PR.

2. **Problem 2** - the implementation of `usleep` on the ESP-IDF can overflow if the passed number of microseconds is >= `u32::MAX - 1_000_000`

This is also addressed by this PR.

Extra details for Problem 2:

`u32::MAX - 1_000_000` is chosen to accommodate for the longest possible systick on the ESP IDF which is 1000ms.

The systick duration is selected when compiling the ESP IDF FreeRTOS task scheduler itself, so we can't know it from within `STD`. The default systick duration is 10ms, and might be lowered down to 1ms. (Making it longer I have never seen, but in theory it can go up to a 1000ms max, even if obviously a one second systick is unrealistic - but we are paranoid in the PR.)

While the overflow is reported upstream in the ESP IDF repo[^1], I still believe we should workaround it in the Rust wrappers as well, because it might take time until it is fixed, and they might not fix it for all released ESP IDF versions.

For big durations, rather than calling `usleep` repeatedly on the ESP-IDF in chunks of `u32::MAX - 1_000_000`us, it might make sense to call instead with 1_000_000us (one second) as this is the max period that seems to be agreed upon as a safe max period in the `usleep` POSIX spec. On the other hand, that might introduce less precision (as we need to call more times `usleep` in a loop) and, we would be fighting a theoretical problem only, as I have big doubts the ESP IDF will stop supporting durations higher than 1_000_000us - ever - because of backwards compatibility with code which already calls `usleep` on the ESP IDF with bigger durations.

[^1]: espressif/esp-idf#14390
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Aug 21, 2024
Rollup merge of rust-lang#129232 - ivmarkov:master, r=workingjubilee

Fix `thread::sleep` Duration-handling for ESP-IDF

Addresses the ESP-IDF specific aspect of rust-lang#129212

#### A short summary of the problems addressed by this PR:
================================================

1. **Problem 1** - the current implementation of `std::thread::sleep` does not properly round up the passed `Duration`

As per the documentation of `std::thread::sleep`, the implementation should sleep _at least_ for the provided duration, but not less. Since the minimum supported resolution of the `usleep` syscall which is used with ESP-IDF is one microsecond, this means that we need to round-up any sub-microsecond nanos to one microsecond. Moreover, in the edge case where the user had passed a duration of < 1000 nanos (i.e. less than one microsecond), the current implementation will _not_ sleep _at all_.

This is addressed by this PR.

2. **Problem 2** - the implementation of `usleep` on the ESP-IDF can overflow if the passed number of microseconds is >= `u32::MAX - 1_000_000`

This is also addressed by this PR.

Extra details for Problem 2:

`u32::MAX - 1_000_000` is chosen to accommodate for the longest possible systick on the ESP IDF which is 1000ms.

The systick duration is selected when compiling the ESP IDF FreeRTOS task scheduler itself, so we can't know it from within `STD`. The default systick duration is 10ms, and might be lowered down to 1ms. (Making it longer I have never seen, but in theory it can go up to a 1000ms max, even if obviously a one second systick is unrealistic - but we are paranoid in the PR.)

While the overflow is reported upstream in the ESP IDF repo[^1], I still believe we should workaround it in the Rust wrappers as well, because it might take time until it is fixed, and they might not fix it for all released ESP IDF versions.

For big durations, rather than calling `usleep` repeatedly on the ESP-IDF in chunks of `u32::MAX - 1_000_000`us, it might make sense to call instead with 1_000_000us (one second) as this is the max period that seems to be agreed upon as a safe max period in the `usleep` POSIX spec. On the other hand, that might introduce less precision (as we need to call more times `usleep` in a loop) and, we would be fighting a theoretical problem only, as I have big doubts the ESP IDF will stop supporting durations higher than 1_000_000us - ever - because of backwards compatibility with code which already calls `usleep` on the ESP IDF with bigger durations.

[^1]: espressif/esp-idf#14390
@bors bors closed this as completed in d0b3c3a Aug 26, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Aug 26, 2024
Rollup merge of rust-lang#129588 - hermit-os:sleep-micros, r=workingjubilee

pal/hermit: correctly round up microseconds in `Thread::sleep`

This fixes the Hermit-related part of rust-lang#129212 and thus the whole issue, since ESP-IDF is already fixed, as far as I understand.

Fixes rust-lang#129212

r? `@workingjubilee`

CC: `@stlankes`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-thread Area: `std::thread` C-bug Category: This is a bug. O-ESP-IDF Target: Espressif IoT Development Framework O-hermit Operating System: Hermit T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants