feat(http/retry): model PeekTrailersBody<B> with Frame<T>

[cratelyn]

this branch contains a sequence of commits that refactor PeekTrailersBody<B>.

this branch is specifically focused on making this body middleware
forward-compatible with the 1.0 interface(s) of http_body::Body and
http_body_util::BodyExt.

it does this in two main steps: (1) temporarily vendoring http_body::Frame<T>
and providing a compatibility shim that provides a frame() method for a body,
and (2) modeling PeekTrailersBody<B> and its peeking logic in terms of this
Frame<'a, T> future.

feat(http/retry): add Frame<T> compatibility facilities

this commit introduces a compat submodule to linkerd-http-retry.

this helps us frontrun the task of replacing all of the finicky control flow in
PeekTrailersBody<B> using the antiquated data() and trailers() future
combinators. instead, we can perform our peeking in terms of an approximation
of http_body_util::BodyExt::frame().

to accomplish this, this commit vendors a copy of the Frame<T> type. we can
use this to preemptively model our peek body in terms of this type, and move to
the "real" version of it when we're upgrading in pr #3504.

additionally, this commit includes a type called ForwardCompatibleBody<B>,
and a variant of the Frame<'a, T> combinator. these are a bit boilerplate-y,
admittedly, but the pleasant part of this is that we have, in effect, migrated
the trickiest body middleware in advance of #3504. once we upgrade to http-body
1.0, all of these types can be removed.

• https://docs.rs/http-body-util/latest/http_body_util/trait.BodyExt.html#method.frame
• https://docs.rs/http-body-util/0.1.2/src/http_body_util/combinators/frame.rs.html#10

Signed-off-by: katelyn martin [email protected]

refactor(http/retry): PeekTrailersBody<B> uses BodyExt::frame()

this commit reworks PeekTrailersBody<B>.

the most important goal here is replacing the control flow of read_body(),
which polls a body using BodyExt future combinators data() and frame()
for up to two frames, with varying levels of persistence depending on outcomes.

to quote #3556:

the intent of this type is to only yield the asynchronous task
responsible for reading the body once. depending on what the inner
body yields, and when, this can result in combinations of: no data
frames and no trailers, no data frames with trailers, one data frame
and no trailers, one data frame with trailers, or two data frames.
moreover, depending on which of these are yielded, the body will call
.await some scenarios, and only poll functions once in others.

migrating this to the Frame and poll_frame() style of the 1.0 Body
interface, away from the 0.4 interface that provides distinct
poll_data() and poll_trailers() methods, is fundamentally tricky.

this means that PeekTrailersBody<B> is notably difficult to port across the
http-body 0.4/1.0 upgrade boundary.

this body middleware must navigate a number of edge conditions, and once it
has obtained a Frame<T>, make use of conversion methods to ascertain
whether it is a data or trailers frame, due to the fact that its internal enum
representation is not exposed publicly. one it has done all of that, it must do
the same thing once more to examine the second frame.

this commit uses the compatibility facilities and backported Frame<T>
introduced in the previous commit, and rewrites this control flow using a form
of the BodyExt::frame() combinator.

this means that this middleware is forward-compatible with http-body 1.0, which
will dramatically simplify the remaining migration work to be done in #3504.

see linkerd/linkerd2#8733 for more information and
other links related to this ongoing migration work.

refactor(http/retry): mock body enforces poll_trailers() contract

this commit addresses a TODO note, and tightens the enforcement of a rule
defined by the v0.4 signature of the Body trait.

this commit changes the mock body type, used in tests, so that it will panic if
the caller improperly polls for a trailers frame before the final data frame
has been yielded.

previously, a comment indicated that we were "fairly sure" this was okay. while
that may have been acceptable in practice, the changes in the previous commit
mean that we now properly respect these rules.

thus, a panic can be placed here, to enforce that "[is] only be called once
poll_data() returns None", per the documentation.

[cratelyn]

💬 breadcrumbs for review...

[cratelyn]

i'm unsure how much to worry about the commented out //Unknown(Box<dyn Frameish>), variant in the vendored frame code. in other words, might this someday be reachable?

Result<T, E> gives us something we can pattern match on, which Frame<T> does not for the sake of semantic versioning. if we fear hitting this in the future, we could define a local enum like so...

enum ParsedFrame<T> {
    Data(T),
    Trailers(HeaderMap),
    Other(Frame<T>),
}

...and return that here. what do you think?

[olix0r]

Or this could be Option<Either<B::Data, HeaderMap>>, and then the caller can be adapted so this is handled as an Empty body... Since the API specifically doesn't give us exhaustive matching it's probably best to handle it cleanly.

[cratelyn]

agreed. this is addressed in 501b3cf

[olix0r]

This is really cool! A few suggestions

[olix0r]

Or this could be Option<Either<B::Data, HeaderMap>>, and then the caller can be adapted so this is handled as an Empty body... Since the API specifically doesn't give us exhaustive matching it's probably best to handle it cleanly.

[cratelyn]

@olix0r, review comments have been addressed! i'd love if you could take a quick look before i hit the merge button 🙂

[olix0r]

looks great!