Disarming Service after successful poll_ready

[mitsuhiko]

Currently if poll_ready returns Ready it effectively reserves something (for instance a semaphore token). This means you must be following up with call next. The only other option is to Drop the service which however is not always possible.

It was mentioned in the tokio repo that it would be useful to be able to disarm a service (tokio-rs/tokio#898).

This would be particularly useful when one wants to implement an HTTP health check for a load balancer by checking poll_ready without actually calling.

Example:

pub trait Service<Request> where
    <Self::Future as Future>::Output == Result<Self::Response, Self::Error>, {
    type Response;
    type Error;
    type Future: Future;
    fn poll_ready(&mut self, cx: &mut Context) -> Poll<Result<(), Self::Error>>;
    fn call(&mut self, req: Request) -> Self::Future;
    fn disarm_ready(&mut self) {}
}

And then be used like this:

pub trait HealthcheckServiceExt<Request>: Service<Request> {
    fn is_healthy(&mut self) -> bool {
        if self.poll_ready().is_ready() {
            self.disarm_ready();
            true
        } else {
            false
        }
    }
}

The proposed new flow would be the following:

• poll_ready must be called. If it's Ready it must be followed up with either call or disarm_ready.
• call must not be called without poll_ready being called before
• disarm_ready may always be called and should undo what poll_ready did
• Clone is supposed to either internally call disarm_ready or do the same it does (a Clone should always be disarmed)

The latter is already the logic that exists in some layers.

Now obviously it's a bit suboptimal that it means not all services will be able to disarmed so it might also be possible to indicate if something can actually be disarmed. For instance already existing services might just not ever be disarmable until they get retrofitted.

[LucioFranco]

@mitsuhiko hey sorry for the late reply here, I think this is some potential here. By chance have you played around with an API like this yet?

[jonhoo]

I came up with a common use-case for this in tokio-tower that I described on Discord, and figured it'd be useful to re-iterate here for posterity.

The issue arises in the context of non-buffering wrappers that are generic over Service. Consider the following code:

impl Future {
  fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
    loop {
      ready!(self.service.poll_ready(cx)?);
      let next = ready!(self.requests.poll_next(cx));
      self.service.start_send(next)?;
    }
  }
}

Great, no buffering! The challenge arises in the contract of poll_ready, which requires that once poll_ready returns Ready, it must continue to do so (or error). As an example, consider tower_buffer::Handle, which is really just a wrapper around tokio::sync::mpsc::Sender. Its poll_ready calls Sender::poll_ready (https://github.com/tokio-rs/tokio/blob/1eb6131321b5bb8e0ccdb7b9433f6f0ef47821f2/tokio/src/sync/mpsc/bounded.rs#L200), which internally "reserves" a slot (https://github.com/tokio-rs/tokio/blob/1eb6131321b5bb8e0ccdb7b9433f6f0ef47821f2/tokio/src/sync/mpsc/chan.rs#L190) for the next send so that it can uphold the contract for poll_ready. The problem arises in the code above when poll_ready returns Ready, but poll_next returns Pending. We are then still "holding up" one poll_ready slot even though we may not use it for a very long time (or ever). If there are enough copies of the tower_buffer::Handle around, they may collectively be holding up all the slots in the bounded channel, so no other requests can go through.

What we need (as observed in this issue) is some way to "give up" ("disarm" seems like a good name for it) a poll_ready if we decide we did not need it after all. In the specific case of tower_buffer::Handle in the code above, we can work around this by cloning the Handle, dropping the old one, and replacing it with the clone to give up the slot when poll_next returns Pending, but that solution does not work if you are generic over S: Service. We also cannot fix this "internally" in tower_buffer, since we have no way of knowing if the caller decides to give up the ready.

[jonhoo]

Related issue for Sink::poll_ready: rust-lang/futures-rs#2109

[LucioFranco]

I agree that this is important but I am struggling to see how we could get this out into a trait easily. I also worry about our current users of tower like hyper which would require an entire ecosystem bump again.

[jonhoo]

I'll add to this that there is only a small number of impl Service whose poll_ready actually does some kind of reservation. Off the top of my head: anything semaphore based (lock/rwlock) and anything based on a bounded channel (buffer, mpsc::Sender). I don't know that that observation helps, but figured I'd put it out there.

[mitsuhiko]

@LucioFranco I ran into this again and came back to this issue. You're saying there is a concern in retrofitting this without breaking the ecosystem. Since a disarm could have an empty default implementation wouldn't it be a somewhat safe change if nobody implements this for a while?

[LucioFranco]

@mitsuhiko the issue with a default is that with composition they would need to forward the inner implementation which is impossible and error prone to do with a default impl. That said, I think we could start exploring a new trait on nightly w/ this addition. The ecosystem is getting in a solid spot now that we can focus on the next iteration.

cc @davidpdrsn

[davidpdrsn]

That said, I think we could start exploring a new trait on nightly w/ this addition. The ecosystem is getting in a solid spot now that we can focus on the next iteration.

I agree! Would be cool to starting thinking about concrete designs.