Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rethink relationship between Future and StableFuture #949

Closed
Nemo157 opened this issue Apr 6, 2018 · 1 comment
Closed

Rethink relationship between Future and StableFuture #949

Nemo157 opened this issue Apr 6, 2018 · 1 comment

Comments

@Nemo157
Copy link
Member

Nemo157 commented Apr 6, 2018

For reference, the current APIs

impl StableFuture {
    fn pin<'a>(self)
      -> PinBox<Future<...> + Send + 'a>
      where Self: Send + 'a
    { ... }

    fn pin_local<'a>(self)
      -> PinBox<Future<...> + Send + 'a>
      where Self: 'a
    { ... }
}

impl<F: Future> StableFuture for F { ... }

These APIs made sense with the original Pin and async/await APIs but there have been two changes since that I believe mean we can rethink these:

  1. await now goes via Future::poll for #[async_move]
  2. Pin and PinBox are both Unpin

Together these suggest an alternative design for the relationship between StableFuture and Future that is more "correct", has no unsafe code, and should support a superset of the operations currently supported:

impl StableFuture {
    fn pin<'a>(self)
      -> PinBox<StableFuture<...> + Send + 'a>
      where Self: Send + 'a
    { ... }

    fn pin_local<'a>(self)
      -> PinBox<StableFuture<...> + Send + 'a>
      where Self: 'a
    { ... }
}

impl<F: StableFuture + Unpin> Future for F { ... }

impl<F: StableFuture + ?Sized> StableFuture for Pin<F> { ... }

impl<F: StableFuture + ?Sized> StableFuture for PinBox<F> { ... }

Previously await! inside #[async_move] used StableFuture::poll, this was the original reason for adding the impl<F: Future> StableFuture. Now that it uses Future::poll and users must pin any StableFuture via something like await!(foo.pin())? this isn't necessary for this use case.

Instead all that is necessary is to acquire something that implements Future from a StableFuture. Currently that can be done only via StableFuture::{pin,pin_local} which internally uses an unsafe layer to implement Future on top of a StableFuture.

We can change StableFuture::{pin,pin_local} to instead return the more "correct" (and safe) PinBox<StableFuture ...> (or drop these functions and have users just call PinBox::new themselves) but still have this : Future via adding the three safe implementations shown above.

The first uses the property that for<T: Unpin> Pin<T> == &mut T, therefore a safe implementation of Future for any type that implements StableFuture can be added. The second and third are just the standard forwarding implementations of object safe traits (using Pin and PinBox instead of the normal &mut and Box). Combined these result in PinBox<StableFuture>: Future and can be used directly in await! inside #[async_move].


I believe these changes can support more ergonomic use of StableFutures in some cases (mostly relating to stack pinning), unfortunately because of the special case StableFuture::pin all examples I have come up with are possible today by just treating the pinned future as a Future rather than a StableFuture, but these changes will allow the same examples to work via a generic stack pinning API once one is defined. One example of the sort of function that is impossible to write without the current special case:

fn next<S: StableStream>(stream: S)
-> impl StableFuture<Item = Option<S::Item>, Error = S::Error>
{ ... }

fn select<L: StableFuture, R: StableFuture>(left: L, right: R)
-> impl StableFuture<
    Item = Either<L::Item, R::Item>,
    Error = Either<L::Error, R::Error>,
>
{ ... }

// Above are just some utility adaptors that are used.

struct Message {
    id: String,
    data: String,
}

fn delay(time: Duration) -> impl StableFuture<Item = (), Error = !>
{ ... }

fn listen() -> impl StableStream<Item = Message, Error = !>
{ ... }

#[async]
fn listen_for(id: &str, timeout: Duration) -> Result<Option<Message>, !> {
    let mut timeout = delay(timeout).pin();
    let mut stream = listen().pin();

    loop {
        match await!(select(timeout.as_pin(), next(stream.as_pin())))? {
            Either::Left(()) => {
                return Ok(None);
            }
            Either::Right(Some(msg)) => {
                if msg == id {
                    return Ok(Some(msg));
                }
            }
        }
    }
}

This example relies on the forwarding implementation on Pin to allow reusing an existing timeout and stream until the timeout expires or a specific message is seen on the stream. This can be trivially changed to use a stack pinning API given one that works in #[async] context by changing the first two lines and updating timeout.as_pin() to Pin::borrow(&mut timeout) and same for the stream.


Obviously this all extends directly to StableStream as well.

I plan to try making this change this weekend and see if I can find any issues with it. I do feel like removing the impl<F: Future> StableFuture for F could be a slight problem in some cases, but unfortunately that collides with impl<F: StableFuture> StableFuture for Pin<F> (unless having StableFuture defined in futures-core might actually allow this, I doubt it though).

I just saw the async/await and companion RFCs after writing this. I think this is heading more in the direction that those are planning as well, the forwarding implementations are definitely going to be wanted once Future is gone.

CC @withoutboats

@Nemo157
Copy link
Member Author

Nemo157 commented Apr 9, 2018

So, my experiment did not go well, having impl<F: Future> Future for &mut F blocks having impl<F: StableFuture + Unpin> Future for F, and PinBox<StableFuture> is blocked on the object safe self types improvements. So I'm abandoning this change and will just have to put up with no impl<F: StableFuture> for Pin<F> until 0.3.

@Nemo157 Nemo157 closed this as completed Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant