From e7973aa15a9e2b8ff322b0d2f89d480ac698806b Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Tue, 7 Jan 2025 00:00:00 +0000 Subject: [PATCH] refactor(http/retry): `PeekTrailersBody` only peeks empty bodies this commit makes a small, subtle change to the `PeekTrailersBody` 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` 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 7f817b574: ``` // 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`: 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 https://github.com/linkerd/linkerd2/issues/8733. see https://github.com/linkerd/linkerd2-proxy/pull/3504. Signed-off-by: katelyn martin --- linkerd/http/retry/src/peek_trailers.rs | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/linkerd/http/retry/src/peek_trailers.rs b/linkerd/http/retry/src/peek_trailers.rs index 7e831a2652..667265b245 100644 --- a/linkerd/http/retry/src/peek_trailers.rs +++ b/linkerd/http/retry/src/peek_trailers.rs @@ -26,7 +26,7 @@ pub struct PeekTrailersBody { first_data: Option>, /// 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 @@ -98,16 +98,6 @@ impl PeekTrailersBody { 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...