Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Chore: Move OnUpgrade state from Body to Request/Response #2353

Merged
merged 1 commit into from
Dec 15, 2020

Conversation

aknuds1
Copy link
Contributor

@aknuds1 aknuds1 commented Dec 6, 2020

Move OnUpgrade state from Body to http::Request/http::Response extensions.

Fixes #2340.

@aknuds1
Copy link
Contributor Author

aknuds1 commented Dec 12, 2020

@seanmonstar I came up with a scheme that I think makes sense, storing OnUpgrade state in MessageHead::extensions and moving the state into Response::extensions and Request::extensions. I get a compilation error due to missing trait implementations for Extensions however, could you help me understand how to solve them please? Also, just point out if I should simply have solved it differently :)

✗ cargo build --all-features
   Compiling hyper v0.14.0-dev (/home/arve/Projects/hyper)
error[E0277]: the trait bound `Extensions: Clone` is not satisfied
  --> src/proto/mod.rs:29:5
   |
29 |     extensions: http::Extensions,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Clone` is not implemented for `Extensions`
   |
   = note: required by `clone`
   = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0369]: binary operation `==` cannot be applied to type `Extensions`
  --> src/proto/mod.rs:29:5
   |
29 |     extensions: http::Extensions,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0369]: binary operation `!=` cannot be applied to type `Extensions`
  --> src/proto/mod.rs:29:5
   |
29 |     extensions: http::Extensions,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

@seanmonstar
Copy link
Member

Those are probably from derives on MessageHead. You can likely remove the ones that are causing a problem (Clone, PartialEq), I don't think they're used any more.

@aknuds1
Copy link
Contributor Author

aknuds1 commented Dec 13, 2020

@seanmonstar Thanks, I thought those traits were necessary. Removed them, fixed the compile :)

@aknuds1
Copy link
Contributor Author

aknuds1 commented Dec 13, 2020

I'll rebase to fix commit messages, since everything else looks fine.

@aknuds1 aknuds1 force-pushed the chore/2340 branch 2 times, most recently from 63c883a to 6a22a2f Compare December 13, 2020 09:31
@aknuds1 aknuds1 marked this pull request as ready for review December 13, 2020 09:33
@aknuds1 aknuds1 requested a review from seanmonstar December 13, 2020 09:40
@aknuds1 aknuds1 force-pushed the chore/2340 branch 2 times, most recently from ca8d55b to 60238f9 Compare December 13, 2020 09:49
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.

Fantastic work! This is exactly what I had in mind. Just one comment inline.

Move state required for protocol upgrades to head
representations, instead of associating it with the body.

Closes hyperium#2340.

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@aknuds1
Copy link
Contributor Author

aknuds1 commented Dec 15, 2020

Thanks @seanmonstar! I applied your suggestion.

BTW, I amended into the original commit, and rebased. Do you prefer this way of working on PRs, or would you prefer if I avoid rewriting the commit history (i.e., just merge, add new commits)?

@aknuds1 aknuds1 requested a review from seanmonstar December 15, 2020 06:57
@seanmonstar seanmonstar merged commit ede3a6b into hyperium:master Dec 15, 2020
@aknuds1 aknuds1 deleted the chore/2340 branch December 15, 2020 15:31
BenxiangGe pushed a commit to BenxiangGe/hyper that referenced this pull request Jul 26, 2021
Move state required for protocol upgrades to head
representations, instead of associating it with the body.

Closes hyperium#2340.

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor HTTP upgrades into Extensions
2 participants