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

refactor: move away from legacy hyper::body::HttpBody #3467

Merged
merged 3 commits into from
Dec 17, 2024

Conversation

cratelyn
Copy link
Collaborator

@cratelyn cratelyn commented Dec 16, 2024

this is an incremental step away from hyper 0.14's request and response
body interfaces, and towards the 1.0 body types. see
linkerd/linkerd2#8733 for more information
about upgrading to hyper 1.0.

hyper 0.14 provides a hyper::body::Body that is removed in the 1.0
interface. hyper-util now provides a workable default body type. hyper
0.14 reëxports http_body::Body as HttpBody. hyper 1.0 reëxports this
trait as hyper::body::Body without any renaming.

this commit moves application code away from hyper's legacy Body type
and the HttpBody trait alias. this commit moves assorted interfaces
towards the boxed BoxBody type instead. when possible, code is tweaked
such that it refers to the reëxport in linkerd-proxy-http, rather than
directly through hyper.

NB: this commit is based upon #3466.

@cratelyn cratelyn force-pushed the kate/hyper-1.x-use-http-body branch from 0caacfa to cac5c3a Compare December 16, 2024 20:36
@cratelyn cratelyn changed the title refactor: move towards new Body interfaces refactor: move away from legacy hyper::body::HttpBody Dec 16, 2024
B: Default + hyper::body::HttpBody,
B: Default + linkerd_proxy_http::Body,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☝️ an example of replacing hyper::body::HttpBody with a stable linkerd_proxy_http::Body.

pub fn serve<B>(&self, req: http::Request<B>) -> std::io::Result<http::Response<Body>> {
pub fn serve<B>(&self, req: http::Request<B>) -> std::io::Result<http::Response<BoxBody>> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☝️ see places like this as well, where we move interfaces towards BoxBody.

@cratelyn cratelyn force-pushed the kate/hyper-1.x-use-http-body branch from cac5c3a to 9b6f0f2 Compare December 16, 2024 20:46
@cratelyn cratelyn marked this pull request as ready for review December 16, 2024 20:58
@cratelyn cratelyn requested a review from a team as a code owner December 16, 2024 20:58
Base automatically changed from kate/http-workspace-dependency to main December 17, 2024 12:46
@cratelyn cratelyn force-pushed the kate/hyper-1.x-use-http-body branch from 9b6f0f2 to cc80a41 Compare December 17, 2024 15:41
linkerd/app/admin/src/server.rs Outdated Show resolved Hide resolved
linkerd/app/admin/src/server/json.rs Outdated Show resolved Hide resolved
cratelyn and others added 3 commits December 17, 2024 13:30
this is an incremental step away from hyper 0.14's request and response
body interfaces, and towards the 1.0 body types. see
<linkerd/linkerd2#8733> for more information
about upgrading to hyper 1.0.

hyper 0.14 provides a `hyper::body::Body` that is removed in the 1.0
interface. `hyper-util` now provides a workable default body type. hyper
0.14 reëxports `http_body::Body` as `HttpBody`. hyper 1.0 reëxports this
trait as `hyper::body::Body` without any renaming.

this commit moves application code away from hyper's legacy `Body` type
and the `HttpBody` trait alias. this commit moves assorted interfaces
towards the boxed `BoxBody` type instead. when possible, code is tweaked
such that it refers to the reëxport in `linkerd-proxy-http`, rather than
directly through `hyper`.

NB: this commit is based upon #3466.

Signed-off-by: katelyn martin <[email protected]>
this commit relates to review feedback offered here:
#3467 (comment)

it is slightly ungainly to place a static string into a BoxBody,
something that is a fairly common pattern throughout e.g. our admin
server.

to help nudge the compiler to infer a `B: Body` of `String`, various
code follows a common convention of calling e.g.
`BoxBody::new::<String>("ready\n".into())` or,
`BoxBody::new("ready\n".to_string())`.

this commit helps reduce that boilerplate by adding a `from_static(..)`
constructor that accepts a static string.

```rust
    /// Returns a [`BoxBody`] with the contents of a static string.
    pub fn from_static(body: &'static str) -> Self {
        Self::new(body.to_string())
    }
```

Co-authored-by: Scott Fleener <[email protected]>
Signed-off-by: katelyn martin <[email protected]>
see <#3467 (comment)>.

@sfleen points out this unfortunate part of our diff:

```diff
-           .body(json.into())
+           .body(BoxBody::new(http_body::Full::new(bytes::Bytes::from(json))))
```

this *is* a bit of an unfortunate edge, where boxing up a body feels
especially cumbersome.

this commit takes an attempt at tweaking the function in question so
that this large nested expression reads a bit nicer.

first, to justify why this gets a little more involved: hyper will no
longer provide the `Body` type after 1.0, so we are tasked with
providing our own body. for our purposes, `Full` works because we have a
single chunk of bytes in hand.

in order to create a `Full`, we must provide a `D: Buf`, which can be
satisfied by the following types:
<https://docs.rs/bytes/1.6.0/bytes/buf/trait.Buf.html#foreign-impls>

most simply, we can cast our vector of bytes into a `Bytes`.

with all of this in hand, we can hoist this creation of the body up out
of the `match` arm, and use `Result::map` to apply these constructors in
sequential order:

```rust
    // Serialize the value into JSON, and then place the bytes in a boxed response body.
    let json = serde_json::to_vec(val)
        .map(Bytes::from)
        .map(http_body::Full::new)
        .map(BoxBody::new);
```

Signed-off-by: katelyn martin <[email protected]>
@cratelyn cratelyn force-pushed the kate/hyper-1.x-use-http-body branch from cc80a41 to 4557952 Compare December 17, 2024 19:16
@cratelyn cratelyn enabled auto-merge (squash) December 17, 2024 19:32
@cratelyn
Copy link
Collaborator Author

NB: i just enabled auto-merge 🙂

@cratelyn cratelyn merged commit deb92db into main Dec 17, 2024
18 checks passed
@cratelyn cratelyn deleted the kate/hyper-1.x-use-http-body branch December 17, 2024 19:53
cratelyn added a commit that referenced this pull request Jan 8, 2025
see #8733.

the tests for the replay body use the `hyper::Body` type removed in the
1.0 release.

this commit replaces this with `BoxBody` where possible, and adds
comments with context about how to update code once upgrading to hyper
1.0.

see #3467 and #3468 which added `from_static()` and `empty()`,
respectively.

Signed-off-by: katelyn martin <[email protected]>
cratelyn added a commit that referenced this pull request Jan 10, 2025
see #8733.

the tests for the replay body use the `hyper::Body` type removed in the
1.0 release.

this commit replaces this with `BoxBody` where possible, and adds
comments with context about how to update code once upgrading to hyper
1.0.

see #3467 and #3468 which added `from_static()` and `empty()`,
respectively.

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants