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

util: add a channel body #140

Merged
merged 4 commits into from
Jan 17, 2025
Merged

util: add a channel body #140

merged 4 commits into from
Jan 17, 2025

Conversation

cratelyn
Copy link
Contributor

@cratelyn cratelyn commented Jan 6, 2025

this branch represents a revival of #100, applying changes that were suggested in review. in particular, see 08aba95 and 4fed62f.

changes

use sync::oneshot for error channel

this commit refactors the channel-backed body in #100, changing the
mpsc::Receiver<E> used to transmit an error into a
oneshot::Receiver<E>.

this should improve memory usage, and make the channel a smaller
structure.

in order to achieve this, some minor adjustments are made:

  • use pin projection, projecting pinnedness to the oneshot receiver,
    polling it via core::future::Future::poll(..) to yield a body frame.

  • add Debug bounds were needed.

use &mut self method receivers

this commit refactors the channel-backed body in #100, changing the
signature of send_* methods on the sender to require a mutable
reference.

@cratelyn
Copy link
Contributor Author

cratelyn commented Jan 8, 2025

the msrv action failed here: https://github.com/hyperium/http-body/actions/runs/12635582609/job/35249835531

the relevant bit of our manifest getting in the way is the dep: prefix syntax, noted as being available only after rust 1.60. see the cargo book here: https://doc.rust-lang.org/cargo/reference/features.html#optional-dependencies

4a5b234 removes this syntax, to be backwards compatible with older rust versions.

@cratelyn
Copy link
Contributor Author

cratelyn commented Jan 8, 2025

ah, i just saw that #128 was merged yesterday. i've rebased on top of that, so we can use the dep: syntax in conditional dependencies. nice!

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Thanks for reviving this! I'm excited to see it happen. One thought inline.

http-body-util/src/channel.rs Outdated Show resolved Hide resolved
cratelyn added a commit to cratelyn/http-body that referenced this pull request Jan 10, 2025
see: hyperium#140 (comment)

this commit adds test coverage exposing the bug, and tightens the
pattern used to match frames yielded by the data channel.

now, when the channel is closed, a `None` will flow onwards and poll the
error channel. `None` will be returned when the error channel is closed,
which also indicates that the associated `Sender` has been dropped.
@cratelyn
Copy link
Contributor Author

cratelyn commented Jan 14, 2025

i see the MRSV listed in http-body-util's Cargo.toml as 1.61, here. #128 seems to imply that the MSRV is 1.61.

conversely, the logs in the failing action above mention using the 1.49 toolchain, here:

running `rustup toolchain add 1.49 --no-self-update`

my understanding is that this is because #128 did not update the rust-version field in the http-body crate. see:

rust-version = "1.49"

do you have a preference between: (a) respecting a 1.49 MSRV, restoring the changes in the commit mentioned above, or (b) updating the MSRV of http-body to match http-body-util, and keeping the dep: syntax?

@seanmonstar
Copy link
Member

Ah, interesting. I think that's happened is that cargo hack will check both MSRV versions set, but the error noticed here is because even though it's only compiling http-body, it has the to parse the whole workspace of Cargo.tomls, and it doesn't understand the one in http-body-util. Whereas the actual Rust code isn't compiled in that step, so it works when moving on to check the util crate.

I'm fine with making the http-body MSRV be whatever the minimum is needed to understad dep: syntax.

cratelyn added a commit to cratelyn/http-body that referenced this pull request Jan 15, 2025
this commit updates the MSRV of the `http-body` crate to 1.61.

this brings the `http-body` crate into step with the MSRV of the
`http-body-util` crate.

in hyperium#128, the MSRV of the `http-body-util` crate was update to 1.61.
however, compiling a crate in practice requires being able to parse all
of the package manifests in its workspace. this means that using more
recent features, like `dep:` syntax for optional dependencies included
via cargo features, will not work due to the older `1.49` toolchain
supported by `http-body`.

see the rust reference, here:
<https://doc.rust-lang.org/cargo/reference/features.html#optional-dependencies>

see hyperium#140, which depends on this commit.
cratelyn added a commit to cratelyn/http-body that referenced this pull request Jan 15, 2025
see: hyperium#140 (comment)

this commit adds test coverage exposing the bug, and tightens the
pattern used to match frames yielded by the data channel.

now, when the channel is closed, a `None` will flow onwards and poll the
error channel. `None` will be returned when the error channel is closed,
which also indicates that the associated `Sender` has been dropped.
@cratelyn
Copy link
Contributor Author

i've opened #141, which updates the MSRV, and rebased the commits in this PR to be based upon that change. i've also confirmed locally that cargo hack --rust-version --no-dev-deps check works as expected.

thanks for your help reviewing this, i am very excited to get this over the finish line. 🙂 🏁

seanmonstar pushed a commit that referenced this pull request Jan 15, 2025
this commit updates the MSRV of the `http-body` crate to 1.61.

this brings the `http-body` crate into step with the MSRV of the
`http-body-util` crate.

in #128, the MSRV of the `http-body-util` crate was update to 1.61.
however, compiling a crate in practice requires being able to parse all
of the package manifests in its workspace. this means that using more
recent features, like `dep:` syntax for optional dependencies included
via cargo features, will not work due to the older `1.49` toolchain
supported by `http-body`.

see the rust reference, here:
<https://doc.rust-lang.org/cargo/reference/features.html#optional-dependencies>

see #140, which depends on this commit.
davidpdrsn and others added 4 commits January 15, 2025 12:18
this applies a review suggestion here:
https://github.com/hyperium/http-body/pull/100/files#r1399781061

this commit refactors the channel-backed body in hyperium#100, changing the
`mpsc::Receiver<E>` used to transmit an error into a
`oneshot::Receiver<E>`.

this should improve memory usage, and make the channel a smaller
structure.

in order to achieve this, some minor adjustments are made:

* use pin projection, projecting pinnedness to the oneshot receiver,
  polling it via `core::future::Future::poll(..)` to yield a body frame.

* add `Debug` bounds were needed.

as an alternative, see tokio-rs/tokio#7059, which proposed a
`poll_recv(..)` inherent method for a oneshot channel receiver.
this applies a review suggestion here:
https://github.com/hyperium/http-body/pull/100/files#r1399780355

this commit refactors the channel-backed body in hyperium#100, changing the
signature of `send_*` methods on the sender to require a mutable
reference.
see: hyperium#140 (comment)

this commit adds test coverage exposing the bug, and tightens the
pattern used to match frames yielded by the data channel.

now, when the channel is closed, a `None` will flow onwards and poll the
error channel. `None` will be returned when the error channel is closed,
which also indicates that the associated `Sender` has been dropped.
@cratelyn
Copy link
Contributor Author

♻️ ...and now rebased on master now that #141 is merged!

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Nice!

@seanmonstar seanmonstar merged commit 86fdf00 into hyperium:master Jan 17, 2025
8 checks passed
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.

3 participants