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

chore(deps): upgrade to hyper 1.x #3504

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Conversation

cratelyn
Copy link
Collaborator

@cratelyn cratelyn commented Jan 6, 2025

@cratelyn cratelyn force-pushed the kate/hyper-1.x-upgrade branch from daf57fe to dd6f80f Compare January 7, 2025 19:04
cratelyn added a commit that referenced this pull request 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()`](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 added a commit that referenced this pull request 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()`](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/hyper-1.x-upgrade branch 2 times, most recently from e10b63a to 88aa1c5 Compare January 8, 2025 05:04
note: this commit will not compile, code changes are intentionally
elided from this commit.

this commit upgrades hyper, http, tonic, prost, related dependencies,
and their assorted cargo features.

see <linkerd/linkerd2#8733>.

Signed-off-by: katelyn martin <[email protected]>
this commit represents main as of linkerd/linkerd2-proxy-api#421.

Signed-off-by: katelyn martin <[email protected]>
NOTE: there is a comment noting that the upgrade middleware does not
expect to be cloneable. it is unfortunately, however, at odds with the
new bounds expected of extensions.

so, `Http11Upgrade` is now Clone'able, but a comment is left in place
noting this weakened invariant.

it's worth investigating how upgrades have changed since, in more
detail, but for the current moment we are interested in being
especially conservative about changing behavior, and focusing on api
changes like `Body::poll_frame(..)`.

Signed-off-by: katelyn martin <[email protected]>
a brief note; this commit happened to tickle an unfortunate sharp edge
in `BoxBody` and `Full`'s respective constructors. type inference could
not figure out how to construct the body, so we refrain from boxing the
response body now.

Signed-off-by: katelyn martin <[email protected]>
@cratelyn cratelyn force-pushed the kate/hyper-1.x-upgrade branch from 88aa1c5 to b85b8bd Compare January 8, 2025 22:54
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.

1 participant