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

feat(client): make body reusable in some cases #1197

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joelwurtz
Copy link
Contributor

@joelwurtz joelwurtz commented Feb 2, 2025

This is a first attempt at having a clonable body.

This is mainly done by cloning Bytes which in most cases does not clone the underlying data, but only a pointer associated to it

Making it a draft for the moment as i think there may be a better way for the service request to handle all of this, i'm wondering if maybe we could return the request associated with response, and cloning parts there and body if reusable.

I also want to make a RequestBody::Stream into a RequestBody::Reusable (so we could create a middleware that ensure this everytime so we can reuse body safely in all cases)

@fakeshadow
Copy link
Collaborator

fakeshadow commented Feb 2, 2025

Personally I would prefer a middleware solution for this. There are mainly two benefits for that approach in this case:

simple design without touching too much in the core logic for better modularity.
exposing higher level configs for customization.

For this case I imagine a middleware solution could be like this snipet:

// this is the core issue of a reusable stream that help to determine if a body can be possibly reused
pub(crate) trait LendingStream {
    // the body clone happens inside the trait and depend on the implementor type
    fn poll_next_lending() -> _;

    fn poll_next() -> _;
}

pub struct BoxBody(Box<dyn LendingStream>)

struct AutoBodyMiddleware<S> {
    // config for how large the body we want to keep in memory for possible reuse
    max_size: usizehttp_service: S
}

impl Service<_> for AutoBody<_> {
    async fn call(&self, _) -> _ {
        let (low, up)= req.body().size_hint();
        if up <= self.max_size {
                // the middleware only help deciding if a lending iterator should be triggered.
                body.set_can_be_lend();
        }
    }
}

This is mostly a concept still as there is no mature and well accepted design for lending iterators in Rust. So for now it remains as a possible solution.

@joelwurtz
Copy link
Contributor Author

Will try to look at this when a have time.

And also maybe we could, in this middleware, add an Expect: 100-continue header when body is too large or have an unknown size ? So if the server have an error (like connection has been shutdown) or any other one, we could respond with a specific error that include the original stream, which will allow user (or another middleware) to retry without cloning it ?

@fakeshadow
Copy link
Collaborator

fakeshadow commented Feb 4, 2025

I guess that could be an separate issue. For preserving body we can start by introducing more side effects to the dispatchers and hold onto &mut http::Request<Body> until ownership is required. From there if there is still need for extra middleware we can add them as configs.

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