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): PeekTrailersBody<B> only peeks empty bodies #3509

Closed
wants to merge 1 commit into from

Conversation

cratelyn
Copy link
Collaborator

@cratelyn cratelyn commented Jan 8, 2025

this commit makes a small, subtle change to the PeekTrailersBody<B> http response body middleware.

to help facilitate upgrading to http-body 1.x, we remove some tricky logic that involves Body interfaces that no longer exist after the 0.4 release.

currently, a PeekTrailersBody<B> is not fully consistent about the conditions in which it will peek the trailers of a response body: the inner body is allowed to yield either (a) zero DATA frames, in which case the body will be .await'ed and polled until the trailers are obtained, or (b) one DATA frame, so long as the inner body immediately yields a trailer.

meanwhile, the documentation comment for the type claims:

An HTTP body that allows inspecting the body's trailers, if a
TRAILERS frame was the first frame after the initial headers frame.

we won't have distinct data() and trailers() interfaces in the 1.0 release. we have a single BodyExt::frame() method.

consequently, porting this middleware as-is would be somewhat difficult. we might have to hold two frames, should we receive one frame, now_or_never() the second frame, and discover that we've been provided a second data frame rather than the trailers.

this all runs slightly against the invariants of Body, see this comment originally added in 7f817b5:

// Peek to see if there's immediately a trailers frame, and grab
// it if so. Otherwise, bail.
// XXX(eliza): the documentation for the `http::Body` trait says
// that `poll_trailers` should only be called after `poll_data`
// returns `None`...but, in practice, I'm fairly sure that this just
// means that it *will not return `Ready`* until there are no data
// frames left, which is fine for us here, because we `now_or_never`
// it.

this isn't quite true, as Trailers is just a wrapper calling poll_trailers:
https://docs.rs/http-body/0.4.6/src/http_body/next.rs.html#28-30

so, let's remove this. doing so will make the task of porting this middleware to http-body 1.0 in the short term, and additionally prevents any potential future misbehavior due to inner bodies not handling this eager trailer polling gracefully.

see linkerd/linkerd2#8733.

see #3504.

this commit makes a small, subtle change to the `PeekTrailersBody<B>`
http response body middleware.

to help facilitate upgrading to http-body 1.x, we remove some tricky logic
that involves `Body` interfaces that no longer exist after the 0.4
release.

currently, a `PeekTrailersBody<B>` is not fully consistent about the
conditions in which it will peek the trailers of a response body: the
inner body is allowed to yield _either_ (a) **zero** DATA frames, in which
case the body will be `.await`'ed and polled until the trailers are
obtained, or (b) **one** DATA frame, so long as the inner body
immediately yields a trailer.

meanwhile, the documentation comment for the type claims:

> An HTTP body that allows inspecting the body's trailers, if a
> `TRAILERS` frame was the first frame after the initial headers frame.

we won't have distinct `data()` and `trailers()` interfaces in the 1.0
release. we have a single
[`BodyExt::frame()`](https://docs.rs/http-body-util/latest/http_body_util/trait.BodyExt.html#method.frame)
method.

consequently, porting this middleware as-is would be somewhat difficult.
we might have to hold two frames, should we receive one frame,
`now_or_never()` the second frame, and discover that we've been provided
a second data frame rather than the trailers.

this all runs slightly against the invariants of `Body`, see this
comment originally added in 7f817b5:

```
// Peek to see if there's immediately a trailers frame, and grab
// it if so. Otherwise, bail.
// XXX(eliza): the documentation for the `http::Body` trait says
// that `poll_trailers` should only be called after `poll_data`
// returns `None`...but, in practice, I'm fairly sure that this just
// means that it *will not return `Ready`* until there are no data
// frames left, which is fine for us here, because we `now_or_never`
// it.
```

this isn't quite true, as `Trailers` is just a wrapper calling
`poll_trailers`:
<https://docs.rs/http-body/0.4.6/src/http_body/next.rs.html#28-30>

so, let's remove this. doing so will make the task of porting this
middleware to http-body 1.0 in the short term, and additionally prevents
any potential future misbehavior due to inner bodies not handling this
eager trailer polling gracefully.

see linkerd/linkerd2#8733.

see #3504.

Signed-off-by: katelyn martin <[email protected]>
@cratelyn cratelyn force-pushed the kate/http-retry-peek-trailers-no-frame branch from d06d133 to e7973aa Compare January 8, 2025 04:34
@cratelyn cratelyn marked this pull request as ready for review January 8, 2025 05:09
@cratelyn cratelyn requested a review from a team as a code owner January 8, 2025 05:09
@cratelyn
Copy link
Collaborator Author

cratelyn commented Jan 8, 2025

in conversation with @olix0r today i've gleaned some wisdom that the existing behavior, permitting one DATA frame so long as trailers are immediately available thereafter, is an intentional optimization to account for unary gRPC responses.

in that case, this change may not be the right course of action.

@cratelyn
Copy link
Collaborator Author

cratelyn commented Jan 8, 2025

upon further reflection, i am going to close this. now knowing the implicit background of why we invoke now_or_never() here, i have some ideas about how to proceed. thanks for the insight! 💭 💡

@cratelyn cratelyn closed this Jan 8, 2025
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