Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retry middleware improvements for non-cloneable requests #790

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fraillt
Copy link

@fraillt fraillt commented Aug 21, 2024

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
Copy link

Kayanski commented Oct 30, 2024

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):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants