Skip to content

Commit

Permalink
refactor(http/retry): PeekTrailersBody<B> only peeks empty bodies
Browse files Browse the repository at this point in the history
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]>
  • Loading branch information
cratelyn committed Jan 8, 2025
1 parent b07b0d8 commit d06d133
Showing 1 changed file with 1 addition and 11 deletions.
12 changes: 1 addition & 11 deletions linkerd/http/retry/src/peek_trailers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ pub struct PeekTrailersBody<B: Body = BoxBody> {
first_data: Option<Result<B::Data, B::Error>>,

/// The inner body's trailers, if it was terminated by a `TRAILERS` frame
/// after 0-1 DATA frames, or an error if polling for trailers failed.
/// after 0 DATA frames, or an error if polling for trailers failed.
///
/// Yes, this is a bit of a complex type, so let's break it down:
/// - the outer `Option` indicates whether any trailers were received by
Expand Down Expand Up @@ -98,16 +98,6 @@ impl<B: Body> PeekTrailersBody<B> {
if let Some(data) = body.inner.data().await {
// The body has data; stop waiting for trailers.
body.first_data = Some(data);

// 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.
body.trailers = body.inner.trailers().now_or_never();
} else {
// Okay, `poll_data` has returned `None`, so there are no data
// frames left. Let's see if there's trailers...
Expand Down

0 comments on commit d06d133

Please sign in to comment.