Undeprecate thread::sleep_ms

[matklad]

Hi! Currently, thread::sleep_ms is deprecated in favor of thread::sleep, but I want to argue that _ms variant is really useful and we should un-deprecate it :)

sleep is an odd function, in that it is rarely useful in "real code"; you almost always want to wait for some condition to happen rather than just sleep for some wall-clock time.

However, there are two "not real code" cases where sleep is used a lot:

1. Debugging: it might be useful to insert sleep(a little) to freeze the system in some particular state. For example, it could be useful to wait to be able to attach debugger to some process, or to expose some race condition.

2. Tests: although sleep in tests is in general an awful idea, sometimes there's just no way around it, and in test code such "correct with some luck" code might actually be acceptable. As an example, in Cargo tests we sleep for a second sometimes because mtimes on MacOS have resolution of one second.

Both of this use-cases are write once/read never, and you have the time to sleep as an integer constant. For such use-cases sleep_ms(92) is so much shorter and clearer than sleep(Duration::from_millis(92)).

Moreover, for debugging case you'd really want to avoid adding imports (luckily, we live in a glorious age when IDEs can insert imports for you automatically, but you will forget to remove the import once sleep is deleted), and the difference between ::std::thread::sleep_ms(92) and ::std::thread::sleep(::std::time::Duration::from_millis(92)) is even more pronounced!

I think there are no problems with sleep_ms per se, besides it being non-orthogonal with sleep? It should be fine to add common-case utility functions which are expressed in terms of underling low-level API (case study: println and eprintln). <sarcasm>One potential problem is that someone could try to use sleep_ms to sleep for a given Duration. Luckily, it is excruciatingly non-trivial to extract milliseconds from a Duration, and this should help the user to discover thread::sleep</sarcasm>.

Note: we actually have sleep_ms function in Cargo for tests specifically.

[rust-highfive]

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

[matklad]

r? @rust-lang/libs

[matklad]

Note that #51418 proposes to add Duration to prelude, which could somewhat help the problem. Though I personally would prefer sleep_ms to Duration in the prelude.

[mark-i-m]

I agree with the motivation, I would rather not write the whole sleep(Duration::from_millis). Perhaps one alternative might be to define something like

sleep(92*MILLIS)
sleep(92*MINUTES)
sleep(92*HOURS)

where MILLIS, MINUTES, HOURS are constant singleton values whose types have overloaded multiplication.

[est31]

For reference, this is the deprecation pr: #29604

[withoutboats]

@rfcbot fcp merge

I think @matklad makes a compelling argument. This kind of sleep functionality is common for quick & dirty kind of code, where the type safety of accepting a duration would not be beneficial. I'm even compelled by the argument that we should add sleep_ms to the prelude.

[rfcbot]

Team member @withoutboats has proposed to merge this. The next step is review by the rest of the tagged teams:

• @Kimundi
• @SimonSapin
• @alexcrichton
• @dtolnay
• @sfackler
• @withoutboats

Concerns:

• constants instead (Undeprecate thread::sleep_ms #51610 (comment))
• Duration constants instead (Undeprecate thread::sleep_ms #51610 (comment))
• duration-constants-instead2 (Undeprecate thread::sleep_ms #51610 (comment))
• duration-constants-instead3 (Undeprecate thread::sleep_ms #51610 (comment))

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[SimonSapin]

I don’t mind un-deprecating, but historically we’ve put the bar much higher than this for inclusion in the prelude. I don’t know if they’re formally written down anywhere, but if we want to revisit those inclusion criteria that’s a larger discussion.

[sfackler]

Is this proposed as a one-off special case or should we have a _ms equivalent of every duration-consuming method?

[matklad]

Note that #51418 proposes to add Duration to prelude, which could somewhat help the problem. Though I personally would prefer sleep_ms to Duration in the prelude.

Sorry, I've expressed my thought in a very confusing way. I do not want to add sleep_ms to the prelude. #51418 proposes to add Duration to the prelude, and that should make sleep easier to use. However, I find that undeprecating sleep_ms (without adding it to the prelude) is overall a better solution than adding Duration to the prelude.

Is this proposed as a one-off special case or should we have a _ms equivalent of every duration-consuming method?

As a special case: sleep is special because in the common case you call it with hard-coded integer literal, and not with a Duration which is passed from elsewhere. If there are other functions which are almost always called as foo(Duration::from_millis(92)), it might be a good idea to add foo_ms variants, but I can't recall any other such function.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[ghost]

I've found thread::sleep(Duration::from_millis(x)) to be annoying to type, too.

In the tests for crossbeam-channel, there is a function named ms which is simply a shorthand for Duration::from_millis. This function is then used for passing a Duration argument to thread::sleep, after, and tick. In this file it is used 33 times, and there are many more uses in other tests, too.

If we undeprecate sleep_ms, I'd still need to keep this function because it's useful for specifying timeouts in select!, e.g.:

select! {
    recv(r2, v) => assert!(v.is_none()),
    recv(channel::after(ms(1000))) => panic!(),
}

Therefore, sleep_ms feels like an unsatisfying solution because:

1. It's only useful for putting the current thread to sleep, not in other contexts.
2. It accepts milliseconds but not nanoseconds, seconds, etc.

I find the suggestion by @mark-i-m the most appealing: introducing constants of type Duration named MILLIS, SECONDS, NANOS, and so on. Such constants could then be used as follows:

thread::sleep(100 * MILLIS);
select! {
    recv(r2, v) => assert!(v.is_none()),
    recv(channel::after(1 * SECONDS)) => panic!(),
}

It's worth noting that Go similarly defines a list of constants in the time package, which I find very pleasing to use:

const (
        Nanosecond  Duration = 1
        Microsecond          = 1000 * Nanosecond
        Millisecond          = 1000 * Microsecond
        Second               = 1000 * Millisecond
        Minute               = 60 * Second
        Hour                 = 60 * Minute
)

[SimonSapin]

@stjepang Adding some constants to std::time (and core::time) sounds fine to me. Would you find them satisfactory if they’re not in the prelude and need to be imported separately?

use std::time::MILLISECOND;
use std::thread::sleep;

sleep(92 * MILLISECOND);

(I see that we have impl Mul<u32> for Duration, we might also need to add impl Mul<Duration> for u32.)

[SimonSapin]

@rfcbot concern duration-constants-instead

[ghost]

@SimonSapin Having such constants in the prelude would be nice, but I'd be fine with manual imports, too.

[matklad]

For my use-case of quickly adding a call to sleep specifically, I would say that 92 * MILLISECOND and Duration::from_millis(92) are more or less equivalent: they both require an import from std::time, and they both are long. MILLISECOND is not as long, but I have to hold shift to type it.

I also would like to note that @stjepang 's use-case is exactly the opposite of what I am describing in the issue: in crossbeam channel, timeouts are exactly the thing which is being tested, it is absolutely not a quick & dirty kind of code I have in mind.

That said, I agree that that we also have a problem of "Duration is unergonomic to create", and that constants look like a good solution to it (though they'll be more shouty than Go's ones... EDIT: well, None is a constant, so we could change that...). Another direction for solving this problem is custom literal suffixes: 1m + 30s + 300ms.

[withoutboats]

@rfcbot concern dissensus

I wasnt in the libs team meeting but I'm pretty strongly in favor of merging this undeprecation. I think APIs should only be deprecated because they are strictly inferior to other APIs; I don't understand why we would tell people with certain use cases that they should just ignore the deprecation warnings telling them not to use an API?

[alexcrichton]

This API was stabilized at 1.0 with the explicit intention of being deprecated as soon as we figured out Duration four releases later in 1.4.0 when sleep was stabilized. This was a crutch because we felt we couldn't stabilize Duration at 1.0 and also felt that some form of sleeping was important enough to stabilize at the 1.0 release.

The addition of Duration not only set the standard for all time-taking APIs but solved a number of problems:

• It's clear what unit a Duration-taking API is. It's a duration.
• There's no need to worry about time calculation into a particular unit, durations handle that for you
• Duration is a shared abstraction for a period of time in general

All of these were existing problems with sleep_ms and other *_ms APIs. What if u64 ms overflowed? Calculations and constants had to be defined to pass arguments. Furthermore methods would sometimes take seconds and others would take milliseconds, confusing the situation.

That's all to say that sleep(Duration) I believe is very well motivated and fixes issues we knew about at 1.0 with the sleep_ms API. We decided at 1.0 to pay the cost of having a perma-deprecated API eventually with sleep_ms.

---

Now the argument here is crucially hinged on convenience. This was not a motivational factor in the design for either iteration. The original sleep_ms was done out of necessity and sleep was not introduced with a priority towards ergonomics of invocations.

I personally feel that this is not worth it. This is setting a clear precedent for the ecosystem of "hey Duration isn't ergonomic, you probably want to use it but also have methods that don't use it". That's not the precedent the standard library wants to set. Instead I think we should invest in an alternate route of making the current iteration more ergonomic, benefitting everyone using Duration immediately as a result (rather than only helping the one sleep API here).

Calling sleep also does not seem common enough to warrant reversing the original decision. There's a whole toolbox of one-liners that we could make more convenient in the standard library. For example the venerable macro to debug and inspect variable values. Or perhaps moving logging into the standard library (or things like that). In my mind at least (personally) sleep doesn't make the cut of being worth it.

[shepmaster]

Frameworks like Ruby on Rails add methods to integers for the same purpose. Both routes are possible in Rust:

trait DurationExt {
    fn millis(self) -> std::time::Duration;
}

impl DurationExt for u64 {
    fn millis(self) -> std::time::Duration {
        std::time::Duration::from_millis(self)
    }
}

#[derive(Debug, Copy, Clone)]
struct Millis;

impl std::ops::Mul<Millis> for u64 {
    type Output = std::time::Duration;

    fn mul(self, _other: Millis) -> Self::Output {
        std::time::Duration::from_millis(self)
    }
}

use std::thread::sleep;

fn main() {
    sleep(32.millis());
    sleep(32 * Millis);
}

I'm in favor of something like this to extend the ergonomics to every Duration-taking function, such as those found in networking code.

[SimonSapin]

Both DurationExt and Millis look fine to me, but I think it’s unlikely either of them would be added to the prelude so they’d still need to be imported.

[shepmaster]

unlikely either of them would be added to the prelude

Oh, I wasn't even proposing such, just dumping implementations in case someone wants them.

[SimonSapin]

Sure, I’m just saying the import should be kept in mind when evaluating how ergonomic such a solution would be.

[mark-i-m]

Sure, I’m just saying the import should be kept in mind when evaluating how ergonomic such a solution would be.

In my experience, when I need sleep, it isn't just once but multiple times, so the import is amortized over multiple usages.

[alexcrichton]

@withoutboats do you have thoughts on what's mentioned above? Does it resolve your concern?

[alexcrichton]

ping @withoutboats, this FCP is currently blocked on your concern

[withoutboats]

@rfcbot resolve dissensus

I still think we should accept this PR but don't have the time to hash this out now.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[jjpe]

Another potential way to make something like sleep() more ergonomic is rather than having it take a Duration, it takes anything that can be converted into a Duration:

fn sleep<D: Into<Duration>>(d: D) { 
   // definition here
}

Alternatively it can be argued that perhaps AsRef is more appropriate than Into, but that's a relatively minor nitpick.

But technically it would be a breaking change.

[rfcbot]

The final comment period, with a disposition to close, as per the review above, is now complete.

[TimNN]

Ping from triage! Since the FCP to close is over, without any additional discussion, I'll assume that the original decision stands and close the PR.

[ghost]

I've opened a PR to add duration constants: #57375