tower-rs / tower

async fn(Request) -> Result<Response, Error>
https://docs.rs/tower
MIT License
3.56k stars 281 forks source link

Retry middleware improvements for non-cloneable requests #790

Open fraillt opened 3 months ago

fraillt commented 3 months ago

Hello,

I wasn't sure how to contribute to this project, so I hope this is a right place to start discussing.

Basically this revision solves real problem that I had with tonic, using retry middleware. The core of the problem is that tonic (a library that implements gRPC) uses HTTP2, which is by design is streaming protocol. For this reason it is not possible to "clone" tonic request by having just a reference to it. In order to make request clone-able we need to fully consume it, and then reconstruct original object from consumed bytes.

This revision does exactly that.

  1. consumes original request and stores it's content into new user defined CloneableRequest.
  2. later, it recreates back original request from the contents of CloneableRequest.

There's some more discussion related to this:

I think that it doesn't make sense to solve this issue at tonic side, because it's doesn't make sense for tonic to have cloneable request (image situation where client is streaming multi-gibabyte file, how would you consume it? where would you store it? it's not responsibility of tonic). On the other hand, if you want to have retry mechanism, you must be aware that a request will be copied in order to be able to retry it.

Any feedback is welcome :)

Kayanski commented 4 weeks ago

Hello ! This was very interesting to me and I implemented something very similar inspired off of your work in my application.

However, instead of adding another method in the trait, I simply changed the clone_request definition to the following:

  /// Tries to clone a request before being passed to the inner service.
  ///
  /// If the request can be cloned, it should return the original request and its clone wrapped in [`Some`]
  ///
  /// If the request cannot be cloned simply, it is allowed to drop and reconstruct the original request twice
  ///
  /// If the request cannot be cloned, returns (original_request, [`None`]). Moreover, the retry
  /// function will not be called if the [`None`] is returned.
  fn clone_request(&mut self, req: Req) -> (Req, Option<Req>);

Here's the implementation I propose for tonic (It's the first time I use tonic bodies, so I'm not 100% sure the body consumption is correct here):