feat(http/retry): add a unit test suite to PeekTrailersBody<B>
#3556
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
a note on process: pending approval, my intent is to land this PR as three commits: one containing the docs additions, one containing the refactor tweaks, and one including the test suite. this will avoid conflating all of this in a commit called |
cratelyn
force-pushed
the
kate/http-retry.add-peek-trailers-test-suite
branch
from
c160f63 to
1e1b62e
Compare
olix0r
approved these changes
Jan 22, 2025
|
a snapshot of this branch as originally submitted for review can be found at https://github.com/linkerd/linkerd2-proxy/tree/kate/http-retry.add-peek-trailers-test-suite.archive-pre-squash. compared to |
this is a squashed commit containing the following: --- docs(http/retry): document `PeekTrailersBody::inner` Signed-off-by: katelyn martin <[email protected]> docs(http/retry): document `PeekTrailersBody::peek_trailers()` Signed-off-by: katelyn martin <[email protected]> docs(http/retry): document `PeekTrailersBody::no_trailers()` Signed-off-by: katelyn martin <[email protected]>
this is a squashed commit containing the following: --- refactor(http/retry): decompose `WithPeekTrailersBody<B>` type alias this commit breaks this large type out into two halves. this is a purely cosmetic change. Signed-off-by: katelyn martin <[email protected]> refactor(http/retry): `PeekTrailersBody` is pin projected we must pass our `Pin<T>`'edness down to the inner `B`-typed body for `PeekTrailersBody` to itself implement `http_body::Body`. this commit tweaks the existing code to rely on the `pin-project` library. this generates a `project()` method to pin inner fields whose `poll_data()` and `poll_trailers()` functions we delegate to. this is a noöp change. Signed-off-by: katelyn martin <[email protected]> refactor(http/retry): defer construction of `PeekTrailersBody<B>` this commit refactors the polling logic in `PeekTrailersBody<B>::read_response`. this commit makes some subtle changes with the migration to hyper 1.0 in mind, to make this function more easily portable to the new signature of `http_body::Body`. see linkerd/linkerd2#8733 for more information. this commit defers the `Self` construction of the `PeekTrailersBody` body. this means that the control flow does not need to reach through to e.g. `body.inner` to poll the inner body being peeked. additionally, it provides us with `let` bindings for the first data frame yielded, and the trailers frame yielded thereafter. this is largely cosmetic, but will make it easier to deal with the additional matching we'll need to do when there is a single polling function that yields us `Frame<D>` objects. Signed-off-by: katelyn martin <[email protected]> refactor(http/retry): `PeekTrailersBody` transforms `B` this is a small structural change to the `PeekTrailersBody::read_response()` function to facilitate writing some unit tests. rather than transforming a `Response<B>` into a `Response<PeekTrailersBody<B>>`, we hoist the `Response::into_parts()` and `Response::from_parts()` calls up. `read_response()` is renamed to `read_body()`. Signed-off-by: katelyn martin <[email protected]>
`PeekTrailersBody<B>` contains some subtle edge cases related to the number of DATA frames yielded by the inner body, and how persistent it will be about polling for TRAILERS frames. for example, if it yields a DATA frame, it will not await trailers being available, but it *will* do so if the inner body does not yield a DATA frame. if a DATA frame is yielded, it will check for a TRAILERS frame, but it must be immmediately available. this is all subtle, and particularly subject to change with the upgrade to http-body 1.0's frame-oriented `Body` interface. so, this commit introduces a test suite for `PeekTrailersBody<B>`. it includes assertions to confirm when the peek middleware can and cannot observe the trailers. some `TODO(kate)` comments are left where issues exist. Signed-off-by: katelyn martin <[email protected]>
cratelyn
force-pushed
the
kate/http-retry.add-peek-trailers-test-suite
branch
from
1e1b62e to
9be9042
Compare
hawkw
reviewed
Jan 22, 2025
cratelyn
added a commit
that referenced
this pull request
Jan 23, 2025
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<T> 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. Signed-off-by: katelyn martin <[email protected]>
cratelyn
added a commit
that referenced
this pull request
Jan 24, 2025
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<T> 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. Signed-off-by: katelyn martin <[email protected]> * 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. Signed-off-by: katelyn martin <[email protected]> * refactor(http/retry): rename `PeekTrailersBody::Buffered` <#3559 (comment)> this is a nicer name than `Unknown` for this case. not to mention, we'll want that name shortly to address the possibility of unknown frame variants. Signed-off-by: katelyn martin <[email protected]> * refactor(http/retry): encapsulate `Inner<B>` enum variants this commit makes the inner enum variants private. #3559 (comment) Signed-off-by: katelyn martin <[email protected]> * refactor(http/retry): gracefully ignore unknown frames #3559 (comment) Signed-off-by: katelyn martin <[email protected]> --------- Signed-off-by: katelyn martin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
linkerd/linkerd2#8733 tracks the upgrade to hyper 1.0.
the
PeekTrailersBody<B>body middleware, provided by thelinkerd-http-retrycrate, is uniquely positioned to be adversely affected by the changes to theBodytrait. this type, in order to attempt to peek an innerB-typed body's HTTP/2 trailers, has some subtle and nuanced edge cases.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
.awaitsome scenarios, and only poll functions once in others.migrating this to the
Frame<T>andpoll_frame()style of the 1.0Bodyinterface, away from the 0.4 interface that provides distinctpoll_data()andpoll_trailers()methods, is fundamentally tricky.this branch is most importantly focused on introducing a unit test suite to
PeekTrailersBody<B>, to help provide assurance that our body middleware behaves as expected across this migration boundary.along the way, some documentation is added, and some minor changes are made to the control flow of the polling logic to carve a path for later changes related to polling.