timer: sleep future should become !Unpin

[carllerche]

This would allow using an intrusive-list for the timer.

[Darksonn]

Be aware that it is very commonly used in manual Future and Stream impls since it's the easiest way to hook into Tokio's timer.

[carllerche]

You think that adding Pin<Box<_>> in the docs w/ an example of a manual impl is enough?

[Darksonn]

If we keep it as a named type rather than an async fn, it's probably ok, although then their type becomes !Unpin too unless they box it.

[carllerche]

Is there a reason for it to stay a named type of boxing it is an acceptable strategy? We can document the pattern.

[Darksonn]

If it stays named, boxing is not forced since they can use pin-project.

[zaharidichev]

I am interested in picking that up if it is fine. If so, a bit of guidance will be highly appreciated.

[sfackler]

It should just require the addition of a PhantomPinned field in the Sleep type: https://docs.rs/tokio/0.3.1/src/tokio/time/sleep.rs.html#62-67

[zaharidichev]

I figured that much. My question was more around the transition to using intrusive list for the timer.

[carllerche]

This would be a breaking change and we haven't forked off for 1.0 yet. I also believe @bdonlan is exploring a bigger timer change right now.

[zaharidichev]

I see, that makes sense

[bdonlan]

For 1.0, I would be inclined to have an API that looks a bit like this:

struct Timer { ..., _p: PhantomPinned }

fn sleep(dur: Duration) -> Timer;
fn sleep_until(t: Instant) -> Timer;

struct TimerUnpin { ... }

impl TimerUnpin {
  fn sleep(dur: Duration) -> Self;
  fn sleep_until(t: Instant) -> Self;
}

The reason to do this this way is that it may be possible to avoid boxing even for unpinned timers in some cases. In particular, we can have a single per-task timer that tracks the earliest timeout seen the last time the task was polled. This would allow us to avoid allocation or pinning provided that the timer is polled on a tokio reactor thread (of course, if polled outside the reactor, we must actually create a timer object that we register with whatever reactor it was created on).

[bdonlan]

Upon further thought, avoiding timer state (by associating wakeup times with the task) would require changes in Context. We can't use thread-locals as we might be going through a nested executor, so we'd need a way to interrogate Context for a backchannel to set a wake-up time. Since adding a TimerUnpin is an API-compatible change we could just encourage people to use Box<Pin<Timer>> in the near term.

As for the intrusive list changes, I'm actively working on them at the moment.