timer: introduce delay function shortcut

[Kobzol]

This PR adds a simple delay shortcut to avoid writing Delay::new everywhere.

I noticed that the sleep shortcut defined above my addition is not used anywhere in the repository, should I at least use it in tests (i.e. delay(now() + n) -> sleep(n))?

Suggested by: #1261

[carllerche]

Thanks 👍 Great work.

[carllerche]

looks like this needs a fmt pass. Also, if you are in there, could you make Delay::new private?

[Kobzol]

Should then also other similar things like Timeout be private too?

[carllerche]

I think Timeout still has to be public because it is returned from trait extensions.

[Kobzol]

Ok :) Switched Delay::new to pub(crate) and ran rustfmt.

[Kobzol]

Oops, making Delay::new private broke tokio tests that were using Delay. Do you want to make this breaking change in 0.2? There will of course already be a lot of breaks in this version, but this one seems a bit wasteful to me, as a lot of existing code (including some of my apps) are using Delay::new and this would break them. Since delay is just a very thin "syntax sugar", wouldn't it be better to also keep the classic constructor to allow easier move to 0.2?

[archseer]

Should this be tagged with #[inline]?

[carllerche]

The compiler should be smart enough about that.

[Ralith]

Does rust do any cross-crate inlining of monomorphic functions that aren't explicitly marked whatsoever right now?

[Kobzol]

According to rust-random/rand#472 (comment), it only inlines monomorphic functions only when LTO is used or it's marked explicitly. However, Delay::new wasn't marked inline before, so nothing has really changed, the compiler can inline Delay::new into delay inside the crate and to the outside the situation will stay the same.

I created a binary crate outside of tokio, included tokio-timer, called delay, compiled in release mode and objdump showed that the call to delay is not inlined, but Delay::new was inlined into delay. So the situation should stay as it was before with this PR.

[carllerche]

We can change this in a follow up PR

[carllerche]

This looks good to me, we would just need CI to pass 👍

[Kobzol]

@carllerche Do you really want to have Delay::new private? All the other types have public constructors, people know Delay now and suddenly, it will stop compiling and they will have no idea what to use instead. The Delay type will still be visible though, so it will be pretty weird that they suddenly cannot create an instance of it.

If we change this, shouldn't we also change the other types to make it consistent? At the very least,
we should update the documentation/examples to make it stand out and increase the discoveability.
The most "visible" way of updating the documentation would be to deprecate Delay::new with a message stating that delay should be now used.

[carllerche]

@Kobzol I believe so. I believe that async fn changes the calculus for deciding how to expose APIs. Previously, the most common pattern was manual future implementation. In this case, you would initialize a Delay and store it in a field. i.e. you would most commonly reference the type.

w/ async / await, the pattern becomes:

tokio::timer::delay(Duration::from_millis(100)).await

the type no longer is referenced. In fact, I think it would be possible to get rid of the Delay type entirely and have async fn delay(...).

You are also correct that we should update all other APIs.

[Kobzol]

I see, this will require a lot more changes then. I removed usages of Delay::new where it was easy to replace it with delay. The remaining case is Starve in one of the tests, I'm not sure what is it exactly for (nor how to rewrite it with async/await).

I agree that with async/await, Delay is a good candidate to be rewritten as an async fn, but in the other cases, I'm not sure. I looked at what else is in the timer crate:

• Deadline is deprecated, so we might probably just remove it.
• Timeout could be easily rewritten with select, however it is often conveniently called on other futures (via the FutureExt trait), so I'm not sure if that can be changed now, since async cannot be used in trait functions.
• Interval could be in theory rewritten, but since first-class async fn support for streams is not there yet, I'm not sure if it's worth it (this would be nicely written with an async generator). My attempt at rewriting Interval:

pub fn interval(at: Instant, duration: Duration) -> impl Stream<Item=()> {
    unfold(0, move |i| {
        delay(at + duration * i).map(move |_| Some(((), i + 1)))
    })
}

• Throttle again seems a bit clunky to me to rewrite without async generators.

I can finish the Delay stuff if you give me a hint how to fix the Starve struct (tests/timer.rs:77). I think that this test needs to see inside the Delay struct though.

[carllerche]

@Kobzol we should only use async fn when it makes sense. Your analysis is correct, the other cases don't make sense to switch to async fn and should stay as is 👍

[Kobzol]

Okay, thanks :) The Starve test is causing some problems. I moved it to the module of Delay, so that it has access to Delay::new.
However, it then started to fail (thread 'delay::tests::starving' panicked at 'timer error: timer is shutdown', tokio-timer/src/delay.rs:112:23). Any idea what might be causing it? If I copy-paste it into the external tests directory, where it was before, it passes. The body of the test stayed exactly the same.

[carllerche]

@Kobzol It has to be moved out of the lib into the tests directory.

[Kobzol]

But then it cannot use Delay::new, if that is supposed to be private. With async fn you lose the information about the concrete type.

[carllerche]

@Kobzol it should be possible to rework the test to not have to name it. The simplest option is probably to use poll_fn. You can also update the Starve future to take T: Future.

[Kobzol]

Thank you. I was under the impression that Starve actually called some method on the Delay type, but it indeed works with a general future, therefore the concrete type is not needed. CI is green now.

Btw do you also want to remove Deadline? I can do that in a separate PR.

[carllerche]

Not for this PR, but it looks like sleep and delay do the same thing now... I added a note to #1261 to figure this out. My inclination is to remove sleep in favor of delay.

[Kobzol]

It's not exactly the same, delay will wake at a specified time, sleep will wake at now + specified duration. In my experience sleep is actually much more useful, since usually I want to asynchronously "sleep" for some time from now, rather than asynchronously wake up at some specific moment in the future. Maybe the names of the functions may be changed, but sleep sounds reasonable to me, as it is an async counterpart to an actual thread::sleep.

[carllerche]

ok, we can keep both for now

[carllerche]

@Kobzol Thanks for the follow through and getting this done 👍 Great work.