rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
96.66k stars 12.49k forks source link

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

Closed workingjubilee closed 2 weeks ago

workingjubilee commented 3 weeks ago

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: https://github.com/rust-lang/rust/issues/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 commented 3 weeks ago

First found on espidf:

https://github.com/rust-lang/rust/blob/0f26ee4fd95a1c046582dfb18892f520788e2c2c/library/std/src/sys/pal/unix/thread.rs#L268-L272

cc @ivmarkov @mabezdev @SergioGasquez

workingjubilee commented 3 weeks ago

Same bug: https://github.com/rust-lang/rust/blob/0f26ee4fd95a1c046582dfb18892f520788e2c2c/library/std/src/sys/pal/hermit/thread.rs#L78-L83

cc @stlankes @mkroening

ivmarkov commented 3 weeks ago

First found on espidf:

https://github.com/rust-lang/rust/blob/0f26ee4fd95a1c046582dfb18892f520788e2c2c/library/std/src/sys/pal/unix/thread.rs#L268-L272

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: https://github.com/rust-lang/rust/issues/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 commented 3 weeks ago

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.