smol-rs / async-task

Task abstraction for building executors
Apache License 2.0
380 stars 37 forks source link

`Builder::spawn` should encode the metadata lifetime contract #76

Open jstarks opened 4 months ago

jstarks commented 4 months ago

Considering this from Builder:

pub fn spawn<F, Fut, S>(self, future: F, schedule: S) -> (Runnable<M>, Task<Fut::Output, M>)
where
    F: FnOnce(&M) -> Fut,
    Fut: Future + Send + 'static,
    Fut::Output: Send + 'static,
    S: Schedule<M> + Send + Sync + 'static,
{
    ...
}

My understanding of the contract around metadata is that it is allocated and pinned as part of task allocation, it is guaranteed to remain valid at least as long as the task's future, and it will not be deallocated without being dropped. If these were not intended to be guarantees, it would be strange to have this extra F wrapper at all, because the caller already had access to the metadata in its old location before it was moved into the task.

However, spawn does not reflect these guarantees in the type system.

I don't think the lifetime guarantee can be encoded with Rust today. You need something like:

F: for<'a> FnOnce(&'a M) -> Fut + 'a

But that doesn't work. Maybe something similar could be done with an explicit trait with GAT and RPITIT, though.

But you can encode the fact that the metadata is effectively pinned for its lifetime. And I think this would be a useful thing to do at the next breaking change:

F: FnOnce(Pin<&M>)
notgull commented 4 months ago

I originally intended task metadata that isn't Copy to be most useful in the spawn_unchecked use case. As you mentioned it's difficult to write a safe lifetime in a way that satisfies the proper conditions in Rust's current set of rules.

But that doesn't work. Maybe something similar could be done with an explicit trait with GAT and RPITIT, though.

This might be possible, but we'd have to save it for the next breaking change. I don't want to release smol v3 until futures-core v1.0 is released.

jstarks commented 4 months ago

Yes, that all makes sense. But I want to emphasize that the addition of Pin on the metadata reference will be useful at the next breaking change, even if we can't solve the lifetime issue.

notgull commented 4 months ago

From a theoretical point of view I agree, but I'm not sure what Pin<&M> gets us from a practical point of view. &M can be converted to Pin<&M> without any issues, and vice versa.