time: introduce `sleep` and `sleep_until` functions #2826

alce · 2020-09-08T16:31:46Z

This PR replaces delay_for with sleep and delay_until with sleep_until but does not remove the original functions.

Documentation is changed to make sleep and sleep_until look like the 'original' functions and delay_for and delay_until are aliases.

Because of the way the functions are wired up internally, I don't think tests need to change but would be happy to rename them or add some if considered necessary.

#2770

davidbarsky · 2020-09-08T16:37:45Z

tokio/src/time/delay.rs

    let registration = Registration::new(deadline, Duration::from_millis(0));
    Delay { registration }
 }

+/// Waits until `deadline` is reached. Alias for [`sleep_until`](sleep_until).
+pub fn delay_until(deadline: Instant) -> Delay {


The main branch of Tokio is targetting 0.3, not 0.2. This means that this aliasing isn't strictly necessary unless you want to backport this to 0.2 (personally, I think that's a good idea, but I'll let others weigh in). If this is backported this to 0.2, then I'd add a #[deprecated] attribute to delay_until with the message "delay_until is deprecated and will be removed in Tokio 0.3; use sleep_until instead."

I like the idea of removing them in v0.3 and having it backported to v0.2 with a #[deprecated] on the delay versions.

Yeah, It just wasn't clear to me from the linked issue what should happen to delay_until and delay_for. My reasoning is that is doesn't really hurt to keep both versions in 0.3 and beyond but I'm happy either way.

Ok, so we'll go with the deprecation notice. This means I'll need to change every internal use of these two functions right? @davidbarsky @Darksonn . I mean throughout the entire repo.

If we switch to sleep_* fns, I don't want to maintain an alias.

Regarding backporting to 0.2, I don't have a strong opinion either way. If it were me, I probably wouldn't bother. If others think it is valuable, I will punt to them.

hawkw · 2020-09-08T16:44:20Z

If the goal is to be consistent with the std::thread::sleep API, should these really be in the time module, or should they be moved to task? I'm personally in favor of keeping them in time, but I thought I'd bring that up.

Darksonn · 2020-09-08T16:50:21Z

@hawkw I vote for keeping it in time.

alce · 2020-09-08T16:53:49Z

@hawkw I also like task::sleep better but wasn't sure how that would work with feature flags, so took the easiest path.

alce · 2020-09-08T18:09:17Z

Oh, I misread @hawkw's preference. So we'll keep them in time

alce · 2020-09-08T18:46:23Z

So, I went through all usages of the delay functions and I'm actually inclined to leave them and just provide aliases to sleep variations.

It's probably not that big of a deal but internally there is this recurring concept of delaying some work to start in the future. While the end result of calling sleep is the same, it just doesn't fit with what the code, tests, docs are trying to convey. In any case, I did the replacements if you decide we should go ahead deprecating the delay versions.

hawkw · 2020-09-08T18:47:36Z

Oh, I misread @hawkw's preference. So we'll keep them in time

I do like the consistency with std from putting it in task, but the part that makes me want to keep it in time is that it seems weird to move only sleep to task, but leave all the other time-related APIs (like interval) in time...but those definitely don't make sense in task.

hawkw · 2020-09-08T18:48:34Z

One thing we could do, although this might be even more confusing, is put a task::sleep in task that's just an alias for time::delay_for. I'm not sure if this is a good idea.

alce · 2020-09-08T19:02:29Z

is put a task::sleep in task that's just an alias for time::delay_for.

I think I prefer this option. task::sleep could have it's own documentation block, example and what not and just mention it is actually an alias. This covers the case when users are looking for a thread::sleep parallel. Users with more 'specialized' needs would be directed to the time module, which would remain as it is.

alce · 2020-09-08T19:07:38Z

Just pushed a commit deprecating delay_for so you can look at the diff.

alce · 2020-09-09T20:51:15Z

Thanks @carllerche. I think this is ready. delay_* funcs have been renamed to sleep_* versions, with the idea to remove the old ones in 0.3. For now, I kept aliases in case it's decided to backport. If not, it's just a quick edit to completely remove them in a follow up PR when the decision is made.

Darksonn · 2020-09-13T12:01:16Z

Is this blocked on some decision, or?

davidbarsky · 2020-09-13T14:38:09Z

It might be blocked on a decision. I'm in favor of backporting and deprecating the delay_* functions; we do this in tracing decently often and I think it provides a good user experience.

Darksonn · 2020-09-13T14:45:40Z

I think it makes sense too.

cdmistman · 2020-09-16T23:05:57Z

If the delay_* functions are deprecated and replaced with task::sleep, would it be possible to make a public constructor for time::Delay?

carllerche · 2020-09-17T19:54:36Z

This is a PR against master, which is targeting 0.3. The old fns should just be removed. If the intent is to release a deprecation in 0.2, the best strategy would be to open this PR against the 0.2 branch, merge that, then create a merge PR from 0.2 -> master. This will keep everything in sync.

I also don't see a clear consensus on the final 0.3 API. Is the plan to rename the fns to sleep_* and keep them in tokio::time or move them to tokio::task?

carllerche · 2020-09-17T19:56:07Z

@cdmistman

If the delay_* functions are deprecated and replaced with task::sleep, would it be possible to make a public constructor for time::Delay?

I don't follow the question. These function would be the public constructors to the future? Are you saying the problem is that the return value is not nameable?

cdmistman · 2020-09-17T20:40:20Z

@carllerche

If the delay_* functions are deprecated and replaced with task::sleep, would it be possible to make a public constructor for time::Delay?

I don't follow the question. These function would be the public constructors to the future? Are you saying the problem is that the return value is not nameable?

The issue is more that there would be no immediately obvious way to construct time::Delay... every other struct in time has an apparent constructor (besides Error and Elapsed, which make sense), but if the only constructors for Delay are task::sleep_*, then that doesn't make as much sense...

alce · 2020-09-21T14:56:24Z

@cdmistman this pr just renames time::delay_for -> time::sleep and time::delay_until -> time::sleep_until. These 2 functions are the only way to get a Delay, which still has no public constructor, same as before.

It seems we are a bit stuck. I propose the following, if you agree:

Keep the functions in the time module. I suppose that if it's later considered a good enough QOL improvement, a task::sleep function could be provided that just calls into time::sleep.
Keep the PR against 0.3 and remove the delay_* functions entirely, no deprecation and no backporting.
Give it a couple of days. If there are no objections, I'll update the PR to remove the old functions and we go ahead.

alce · 2020-09-25T15:31:21Z

Removed the delay_* functions as proposed.

Darksonn

I think this is good.

davidbarsky reviewed Sep 8, 2020

View reviewed changes

Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-time Module: tokio/time labels Sep 8, 2020

alce force-pushed the time-sleep branch from a59421d to 4a46982 Compare September 9, 2020 20:44

alce added 5 commits September 25, 2020 10:55

time: introduce sleep and sleep_until functions

4e1c5e1

deprecate delay_for

d7f4677

deprecate delay_until, finish up

5b29c80

remove delay_* functions

e652d74

rebase master

eae5a39

alce force-pushed the time-sleep branch from 3c060b2 to eae5a39 Compare September 25, 2020 16:17

Darksonn requested a review from davidbarsky September 26, 2020 09:48

Darksonn approved these changes Sep 29, 2020

View reviewed changes

davidbarsky approved these changes Sep 29, 2020

View reviewed changes

Darksonn merged commit 53ccfc1 into tokio-rs:master Oct 1, 2020

alce mentioned this pull request Oct 8, 2020

time: rename Delay future to Sleep #2932

Merged

futursolo mentioned this pull request Oct 31, 2021

Make gloo::timer's future-based design like stdlib rustwasm/gloo#162

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

time: introduce `sleep` and `sleep_until` functions #2826

time: introduce `sleep` and `sleep_until` functions #2826

alce commented Sep 8, 2020

davidbarsky Sep 8, 2020

Darksonn Sep 8, 2020

alce Sep 8, 2020

alce Sep 8, 2020 •

edited

carllerche Sep 9, 2020

hawkw commented Sep 8, 2020

Darksonn commented Sep 8, 2020

alce commented Sep 8, 2020

alce commented Sep 8, 2020

alce commented Sep 8, 2020

hawkw commented Sep 8, 2020

hawkw commented Sep 8, 2020

alce commented Sep 8, 2020

alce commented Sep 8, 2020

alce commented Sep 9, 2020

Darksonn commented Sep 13, 2020

davidbarsky commented Sep 13, 2020

Darksonn commented Sep 13, 2020

cdmistman commented Sep 16, 2020

carllerche commented Sep 17, 2020

carllerche commented Sep 17, 2020

cdmistman commented Sep 17, 2020

alce commented Sep 21, 2020

alce commented Sep 25, 2020

Darksonn left a comment

time: introduce sleep and sleep_until functions #2826

time: introduce sleep and sleep_until functions #2826

Conversation

alce commented Sep 8, 2020

davidbarsky Sep 8, 2020

Choose a reason for hiding this comment

Darksonn Sep 8, 2020

Choose a reason for hiding this comment

alce Sep 8, 2020

Choose a reason for hiding this comment

alce Sep 8, 2020 • edited

Choose a reason for hiding this comment

carllerche Sep 9, 2020

Choose a reason for hiding this comment

hawkw commented Sep 8, 2020

Darksonn commented Sep 8, 2020

alce commented Sep 8, 2020

alce commented Sep 8, 2020

alce commented Sep 8, 2020

hawkw commented Sep 8, 2020

hawkw commented Sep 8, 2020

alce commented Sep 8, 2020

alce commented Sep 8, 2020

alce commented Sep 9, 2020

Darksonn commented Sep 13, 2020

davidbarsky commented Sep 13, 2020

Darksonn commented Sep 13, 2020

cdmistman commented Sep 16, 2020

carllerche commented Sep 17, 2020

carllerche commented Sep 17, 2020

cdmistman commented Sep 17, 2020

alce commented Sep 21, 2020

alce commented Sep 25, 2020

Darksonn left a comment

Choose a reason for hiding this comment

time: introduce `sleep` and `sleep_until` functions #2826

time: introduce `sleep` and `sleep_until` functions #2826

alce Sep 8, 2020 •

edited