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

Can the Unpin bound be removed for boxed futures? #1649

Closed
seanmonstar opened this issue Jun 4, 2019 · 4 comments
Closed

Can the Unpin bound be removed for boxed futures? #1649

seanmonstar opened this issue Jun 4, 2019 · 4 comments

Comments

@seanmonstar
Copy link
Contributor

Currently, a Box<Future> only implements Future if it is also Unpin. Is this bound truly required? If it isn't Unpin, you can't safely remove it from a Pin anyways, so you couldn't pin, poll, and Unpin anyways...

@laizy
Copy link
Contributor

laizy commented Jun 4, 2019

I think we can not. because Box<T> is Unpin, even if T is !Unpin.

let mut fut: Box<CanHaveSelfRefFut> = ...;
{
    let pin_fut : Pin<&mut Box<CanHaveSelfRefFut>> = Pin::new(&mut fut); // fut is unpin, so new_unchecked is not needed;
    // call pin_fut.poll, because Box<T> implement `Future`
}

// move out CanHaveSelfRefFut from Box, which is unsafe.

@Nemo157
Copy link
Member

Nemo157 commented Jun 4, 2019

As @laizy mentions this implementation would be unsound, as you could safely move the future out of a Box into a new Box and poll it from multiple memory locations.

One solution would be to extend std::future and .await to use a std::future::IntoFuture trait, then std can safely provide

trait IntoFuture {
    type Future: Future<Output = Self::Output>;
    type Output;
    fn into_future(self) -> Self::Future;
}

impl<F: Future + ?Sized> IntoFuture for Box<F> {
    type Future = Pin<Box<F>>;
    type Output = F::Output;
    fn into_future(self) -> Self::Future { self.into() }
}

Or, the current solution, just use Pin<Box<dyn Future<Output = T>>> everywhere (which futures provides as futures::future::BoxFuture<'_, T> or futures::future::LocalBoxFuture<'_, T> depending on whether you want the + Send bound).

@seanmonstar
Copy link
Contributor Author

Hm, so it seems adding that is unsound, since you can safely call Pin::new(&mut boxed), which is allowed because of the Unpin for Box.

I'm updating things in Tokio, and noticed it looks weird to adjust the spawn functions to require passing a Pin<Box<dyn Future>>, since the executors would be pinning them internally anyways...

@Nemo157
Copy link
Member

Nemo157 commented Jun 4, 2019

Taking an impl IntoFuture for spawn instead of impl Future would fix that as well (if it existed), alternatively as discussed elsewhere (although I can't find a good discussion to link to right now) the goal for futures::task::Spawn is to eventually have the API be fn spawn(&mut self, future: dyn Future<Output = ()> + Send + 'static) once it's possible to take unsized values (some quick testing shows that this API is possible on nightly now, although I have no idea how to then stick that unsized value into a Box inside the executor).

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

3 participants