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.

Sign up for GitHub

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

Jump to bottom

Move HTTP upgrades API off the Body type #2086

Closed
seanmonstar opened this issue Dec 20, 2019 · 3 comments · Fixed by #2337
Closed

Move HTTP upgrades API off the Body type #2086

seanmonstar opened this issue Dec 20, 2019 · 3 comments · Fixed by #2337
Labels
A-http1 Area: HTTP/1 specific. B-breaking-change Blocked: this is an "API breaking change". C-feature Category: feature. This is adding a new feature. E-medium Effort: medium. Some knowledge of how hyper internal works would be useful.
Milestone
0.14

Comments

@seanmonstar
Copy link
Member

Introduced in #1563, the HTTP Upgrade/CONNECT API currently is accessed via hyper::Body::on_upgrade. This has some downsides that I'd like to address with a modified API:

  • It makes the Body type fatter.
  • It requires people keep the Body around, meaning they can't use stream combinators to read the body first, and then wait for the upgrade to finish. This also makes it more annoying for users who may wish adjust their http::Request<Body> into some http::Request<Doodad>.

Proposed API

Similar to the proposal in #1586, the change would be to pass the http::Request or http::Response to a free function.

  • hyper::upgrade::on(req_or_resp) -> OnUpgrade
    • We can also allow passing just a mutable reference, thus not consuming the request or response.
  • Optional other functions could be added, but not necessary: has_upgrade, etc.

Implementation Plan

The types would be inserted into the http::Extensions of the request/response (though with a private wrapper type to hide the exact details).

  • Add the new hyper::upgrade::on function.
  • The Body::on_upgrade method would become #[deprecated].
  • So as to not be a behavioral breaking change, the OnUpgrade would need to be placed in both the Body and the Extensions, wrapped in some Arc<Lock> that only allows taking it once.

I'd like to in a future release be able to not put it in an Arc<Lock> and place it in the Body, but I'm not sure how that could really be done in 0.13.x. (I knew I'd find something I forgot to change after finally releasing XD).
@seanmonstar seanmonstar added A-http1 Area: HTTP/1 specific. B-rfc Blocked: More comments would be useful in determine next steps. C-feature Category: feature. This is adding a new feature. labels Dec 20, 2019
@dekellum
Copy link

dekellum commented Dec 20, 2019 •
edited
Loading

Just my 2¢, and I don't know how you or others would feel about it, but at this early stage it should be fine IMO to just release a 0.14 with it changed to the way you believe it should be. I suspect that only a few of us users have gone far upgrading to 0.13 yet and and its just another version to bump in our Cargo.toml. Said from the opposite direction a quick 0.14 turnover is less concerning than breaking changes in 0.13.4 or whatever.

Maybe 0.13 just wasn't your lucky number?

If I had managed to release anything with an "^0.13" dependency already I might feel a little different, but not much.

@dekellum
Copy link

A related question: should any of this updated API actually go in the http crate?

@VecrsIssues
Copy link

Should support for intermediate 1xx responses also be added?

https://tools.ietf.org/html/rfc7231#section-6.2

This could be useful for 103: Early Hints, or something else added in the future.

@seanmonstar seanmonstar added this to the 0.14 milestone Oct 15, 2020
@seanmonstar seanmonstar added B-breaking-change Blocked: this is an "API breaking change". E-medium Effort: medium. Some knowledge of how hyper internal works would be useful. and removed B-rfc Blocked: More comments would be useful in determine next steps. labels Nov 6, 2020
@seanmonstar seanmonstar mentioned this issue Nov 19, 2020
Merged
seanmonstar added a commit that referenced this issue Nov 20, 2020
@seanmonstar
feat(upgrade): Moved HTTP upgrades off Body to a new API (#2337)
121c331 
Closes #2086

BREAKING CHANGE: The method `Body::on_upgrade()` is gone. It is
  essentially replaced with `hyper::upgrade::on(msg)`.
@seanmonstar seanmonstar mentioned this issue Nov 20, 2020
Closed
@acfoltzer acfoltzer mentioned this issue Feb 8, 2021
Open
BenxiangGe pushed a commit to BenxiangGe/hyper that referenced this issue Jul 26, 2021
@seanmonstar
feat(upgrade): Moved HTTP upgrades off Body to a new API (hyperium#…
54da91b 
…2337)

Closes hyperium#2086

BREAKING CHANGE: The method `Body::on_upgrade()` is gone. It is
  essentially replaced with `hyper::upgrade::on(msg)`.
@luqmana luqmana mentioned this issue Oct 30, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-http1 Area: HTTP/1 specific. B-breaking-change Blocked: this is an "API breaking change". C-feature Category: feature. This is adding a new feature. E-medium Effort: medium. Some knowledge of how hyper internal works would be useful.
Projects
None yet
Milestone
0.14
Development

Successfully merging a pull request may close this issue.

feat(upgrade): Moved HTTP upgrades off Body to a new API hyperium/hyper
3 participants
@seanmonstar @dekellum @VecrsIssues