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

refactor(http/retry): replayed body is not optional #3585

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

Conversation

cratelyn
Copy link
Collaborator

@cratelyn cratelyn commented Jan 31, 2025

ReplayBody<B> is slightly inconsistent about how it represents the exhaustion of the inner B-typed body.

first, in poll_data() we poll the inner body, and note that we consider a None value for BodyState::rest to mean that there is no more work to do.

// Poll the inner body for more data. If the body has ended, remember
// that so that future clones will not try polling it again (as
// described above).
let mut data = {
    // Get access to the initial body. If we don't have access to the
    // inner body, there's no more work to do.
    let rest = match state.rest.as_mut() {
	Some(rest) => rest,
	None => return Poll::Ready(None),
    };
    // ...
};

then, in Body::is_end_stream(), we write the following to check that the stream is done:

let is_inner_eos = self
    .state
    .as_ref()
    .and_then(|state| state.rest.as_ref().map(Body::is_end_stream))
    .unwrap_or(false);

note in particular the call to Option::and_then(..).

this means that, should the inner body be None, we could accidentally begin reporting the stream as not finished. this is backwards!

this commit makes a change to the definition of BodyState<B>. it no longer treats its inner body as optional. we never drop the inner body B, to facilitate gracefully replaying bodies that were interrupted when initially being polled.

a bare B means that various bits of control are now simpler, without having to account for the Option<T> state.

this is front-running other work to port the ReplayBody<B> to http-body 1.0. see linkerd/linkerd2#8733.

`ReplayBody<B>` is slightly inconsistent about how it represents the
exhaustion of the inner `B`-typed body.

first, in `poll_data()` we poll the inner body, and note that we
consider a `None` value for `BodyState::rest` to mean that there is no
more work to do.

```rust
// Poll the inner body for more data. If the body has ended, remember
// that so that future clones will not try polling it again (as
// described above).
let mut data = {
    // Get access to the initial body. If we don't have access to the
    // inner body, there's no more work to do.
    let rest = match state.rest.as_mut() {
	Some(rest) => rest,
	None => return Poll::Ready(None),
    };
    // ...
};
```

then, in `Body::is_end_stream()`, we write the following to check that
the stream is done:

```rust
let is_inner_eos = self
    .state
    .as_ref()
    .and_then(|state| state.rest.as_ref().map(Body::is_end_stream))
    .unwrap_or(false);
```

note in particular the call to `Option::and_then(..)`.

this means that, should the inner body be `None`, we will begin
reporting the stream as **not** finished. this is backwards!

this commit makes a change to the definition of `BodyState<B>`. it no
longer treats its inner body as optional. we never drop the inner body
`B`, to facilitate gracefully replaying bodies that were interrupted
when initially being polled.

a bare `B` means that various bits of control are now simpler, without
having to account for the `Option<T>` state.

Signed-off-by: katelyn martin <[email protected]>
@cratelyn cratelyn marked this pull request as ready for review January 31, 2025 23:02
@cratelyn cratelyn requested a review from a team as a code owner January 31, 2025 23:02
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.

1 participant