Make poll_ready() return a token which call() consumes

[willglynn]

Currently, Service contains:

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

Convention requires that the user call poll_ready(), and only when it returns Poll::Ready(Ok(())) is the user permitted to call call(). call() is permitted to panic if misused.

If we introduce token – an opaque type which the user cannot construct – we can instead say:

trait Service {
  // …
  type Token;
  fn poll_ready(&mut self, cx: &mut Context) -> Poll<Result<Self::Token, Self::Error>>;
  fn call(&mut self, req: Request, token: Self::Token) -> Self::Future
}

Now the type system enforces the required call sequence. The user must call poll_ready() and receive a Poll::Ready(Ok(token)), since call() now requires such a token to accompany each request.

This pattern has benefits beyond just preventing API misuse. The token type could itself hold the resources necessary to handle a request, like a lock on a table slot or a connection checked out from a connection pool. In other words, this proposal reduces the TOCTOU raciness of the poll_ready() -> call() dance.

A token also provides an alternative to the disarming mechanism proposed in #408: a user who got a Poll::Ready(Ok(_)) may either return their token to the service via call(), or they may forfeit the opportunity to make a request by dropping their token. Just as type system can guarantee the correct flow between poll_ready() and call(), the ownership system can guarantee use-it-or-lose-it semantics for every Poll::Ready(Ok(_)) response, ensuring services can all reliably distribute and reclaim per-request resources.

[LucioFranco]

I agree that this API is much more misuse resistant and would enable disarm. That said, I think as of now none of us have the current bandwidth to refactor the code into this. I think we are mostly waiting for GAT to arrive to do the last and final version of tower. So as of now I think we will stick with the current API since, we a lot of use have it deployed already in production 😄. I'd love to see what a PoC of this would look like though!

cc @davidbarsky I know you've brought this one up a lot.

[willglynn]

@LucioFranco This is a pretty mechanical refactor: willglynn@44de76e

All this boils away entirely if Token is a ZST, so it's strictly a safety improvement with no runtime penalty – and the cases where it isn't a ZST, it can be used for new purposes (like disarming) which the current API cannot address.

It also seems to me that making a change like this in tower 0.x is better than deferring it until later. The weight of production systems will only get heavier after a 1.0 release, so if this change is welcome, now is the time to do it.

I'd be happy to continue plumbing this through the other crates if there's interest in merging it.

[LucioFranco]

I think the plan for the next breaking change for tower-service is planned for when GAT arrives.

cc @carllerche id love to hear your thoughts

[najamelan]

Why does this need GAT to be implemented?

update: I found it on discord: only issue is how you tie the token to self to disarm on drop without GAT

[nirvana-msu]

Would be nice to see that implemented, now that GATs are there.

[nirvana-msu]

Current API makes it way too easy to introduce race conditions, and the proposed change with explicitly attaching reserved resource(s) to the token seems like a good way to address this issue.

For instance, in reqwest/issues/491 it was suggested to put RateLimit behind Buffer middleware to allow making concurrent requests from multiple tasks, but unless I'm misunderstanding something, that just leads to a race condition (because RateLimit doesn't actually reserve resources in poll_ready).

Separately, another feature that poll_ready seems to be lacking is allowing to specify exactly which/how many resources should be reserved for the upcoming call. In particular, issues/713 describes a use-case for rate limiting where each request has its own weight. Ideally caller would pass the weigth to poll_ready to allow it to reserve exactly the needed amount of resource. Perhaps this could be taken into account when re-designing the API using tokens,

[damooo]

It's been too tedious to ensure correct usage of current services. This change can make it trivial and rusty in using compiler to ensure proper usage. As GATs are now stable, it would be great if proposal is considered.

[adityabaradwaj]

Now that GATs are stable, this would be a great change that would make tower's idioms more "rust-y"