Pinning and Service

[jonhoo]

Now that we are moving to std::future (#279), we have to make a decision as to how this affects the Service trait. The current Service trait does not mention Pin:

pub trait Service<Request> {
    fn poll_ready(&mut self) -> Poll<(), Self::Error>;
    fn call(&mut self, req: Request) -> Self::Future;
}

Consider what happens in the common case where poll_ready wants to internally poll another future. To do so, it must have a Pin to that future. Since we do not pass Pin<&mut Self> to poll_ready, this means that the Service must internally box such futures (or otherwise cause them to be Unpin) so that Pin::new works. It can't use Pin::new_unchecked, since it cannot guarantee that the service itself isn't moved.

Effectively, this means that we are requiring Service to be Unpin (note that if Service doesn't have any internal futures to poll, then it will trivially be Unpin). This adds some additional "hidden" costs to each service, as they effectively have to box (or otherwise heap-allocate) any internal futures. Ignoring those costs, however, currently, that restriction is implicit in the type signature. We have a few options here as to how to "fix" this.

Option A: keep everything as-is

Implementors of Service will have to discover that they must be Unpin, or will write unsound unsafe code to access fields (because again, Pin::new_unchecked is not okay in this setting).

Option B: add Unpin bound to Service

pub trait Service<Request>: Unpin { /* ... */ }

This makes it explicit that implementors of Service must implement Unpin, but does not actually make it more difficult to implement Service. Adding this bound does have the added benefit that hopefully implementors won't start playing with unsafe Pin::new_unchecked stuff as they will be forced to make their type implement Unpin.

Option C: B, and also make poll_ready take Pin

pub trait Service<Request>: Unpin {
    fn poll_ready(self: Pin<&mut Self>) -> Poll<(), Self::Error>;
    fn call(&mut self, req: Request) -> Self::Future;
}

This makes little practical difference to safety, since we still require Service: Unpin, so the Pin<&mut Self> does nothing (it is always DerefMut), and can be constructed safely with Pin::new. It does have the advantage that the API now looks more like other async APIs where poll methods take Pin. It comes at an ergonomic cost, in that callers must now wrap the Service with Pin::new before calling poll_ready.

It's worth pausing here to observe that we now see why the Unpin bound is really necessary. We want poll_ready to take Pin<&mut Self>, but we want call to take &mut self. This is fundamentally not okay unless the service is Unpin, since call could trivially move self even though it was pinned when poll_ready was called. Which leads us to the final proposal:

Option D: Pin everywhere

pub trait Service<Request> {
    fn poll_ready(self: Pin<&mut Self>) -> Poll<(), Self::Error>;
    fn call(self: Pin<&mut Self>, req: Request) -> Self::Future;
}

This drops the Unpin bound on Service, but comes at a potentially large ergonomic cost: call now requires you to have a Pin to the service. This means that you can no longer just hold on to a Service and poll and call it at will. Instead, you will have to pin it in the caller in order to use it. The caller will then have to either annotate their code with Pin::new, use pin-project, or otherwise arrange for a Pin to the service. The removal of the restriction that Service: Unpin does mean that implementors of Service no longer need to box internal futures though, which may avoid some processing overheads, especially in deep stacked services.

[jonhoo]

Please add to the arguments for/against each option if you have more specific concerns or examples /cc @carllerche @LucioFranco @seanmonstar

[LucioFranco]

Thanks for the write-up @jonhoo!

So I think for now Option C and D are kind of off the table until we see this is actually causing issues. So far I have converted both balance and buffer to std::future and luckily there was no boxing needed. Mostly the reason we didn't need boxing is that FuturesUnordered impls Unpin for any inner Fut.

As for A and B, I am actually leaning with A.

Adding this bound does have the added benefit that hopefully implementors won't start playing with unsafe Pin::new_unchecked stuff as they will be forced to make their type implement Unpin.

So how I see it this actually makes me think that people will just add a blanket impl<T> Unpin for MyType since it is safe, then attempt to use Pin::new_unchecked without realizing they are breaking the pin projection contract. This I think will happen with Service: Unpin or not and for the sake of keeping things lean it might just make sense to not add Service: Unpin. I'd like to hear what others have to say.

To recap so far I have not seen adverse effects of any boxing required to implement a fully Unpin service. So for now I say we keep with option A and document this in the trait docs for Service.