Upgrade to Hyper 1.0 & Axum 0.7 #1670
Conversation
|
I'd like to propose that this be reviewed as-is, and that we add follow-up issues for the remaining todos, which will all require PRs to land and then releases to happen for |
|
works perfectly after |
remove `--ignore` option when pr(when hyperium/tonic#1670) is merged. Signed-off-by: Phoeniix Zhao <Phoenix500526@163.com>
remove `--ignore` option when pr(when hyperium/tonic#1670) is merged. Signed-off-by: Phoeniix Zhao <Phoenix500526@163.com>
examples/Cargo.toml
Outdated
| rustls-pemfile = { version = "1", optional = true } | ||
| tower-http = { version = "0.4", optional = true } | ||
| tokio-rustls = { version = "0.26.0", optional = true } | ||
| hyper-rustls = { version = "0.27.0", features = ["http2"], optional = true } |
There was a problem hiding this comment.
It would be nice if there were feature flags to switch between the crypto backends.
Defaulting to aws-lc-rs pulls in a cmake, nasm requirement, impacts platform support, etc.
Trading that off for FIPS compliance isn't really needed in most cases.
There was a problem hiding this comment.
Which crypto provider tonic should default to is up to the maintainers, I guess.
But reqwest seems to be going the route of sticking to ring as the default crypto backend.
There was a problem hiding this comment.
I think that is a good idea, but I don't think it should be included in this PR - this PR is already complicated as is. We can create an issue and work on multi-backend support separately once this lands.
There was a problem hiding this comment.
Yeah, definitely. I just felt like the comment fit best here for the time being since the PR isn't merged yet and I probably would have forgotten about it if I hadn't written it down
There was a problem hiding this comment.
Following up here - I reverted the default to use ring and not aws-lc-rs for exactly the same reason as reqwest
|
cc @tottoto @LucioFranco can we get this patch a review? This should be one of the last blockers for many downstreams upgrade to hyper 1.0. |
|
@alexrudy FYI hyperium/hyper-util#102 is merged now. |
|
@alexrudy Example h2c failed. called `Result::unwrap()` on an `Err` value: hyper::Error(User(ManualUpgrade)) |
alexrudy
force-pushed
the
hyper-1.0
branch
2 times, most recently
from
5f59197
to
d50ce89
Compare
|
@alexrudy and @ikrivosheev thanks for all your work on this! I'm going to be helping @LucioFranco out with tonic reviews, and obviously this is a desirable change. As such, I've made an initial pass of the changes here. Here are some thoughts:
|
There was a problem hiding this comment.
I am still working through the PR, I think what @djc makes a lot of sense, I am going to continue reviewing the core web and tonic parts since it seems like those changes are somewhat non trivial. So it will take some time just as an FYI.
| @@ -52,7 +52,10 @@ async fn connect_returns_err_via_call_after_connected() { | |||
| let res = client.unary_call(Request::new(Input {})).await; | |||
|
|
|||
| let err = res.unwrap_err(); | |||
| assert_eq!(err.code(), Code::Unavailable); | |||
| assert_eq!(err.code(), Code::Unknown); | |||
There was a problem hiding this comment.
I kinda want to keep the old assert and have the tests failing and get that fixed upstream before we merge. I think keeping these error codes is super super important.
|
@djc Thanks so much for your initial comments. Overall, I tried to minimally scope the changes here based on what was required to make the tests pass with For instance:
I will fix the For This also brings up @LucioFranco's point about the changed error type, which we might want to leave failing: that sounds fine by me, I don't know of a good way though to leave a test in an expected failure state in Rust. Also, just letting the test fail will be problematic, as CI is currently configured to fail-fast, meaning that one failure will cause a lot of other tests to be skipped entirely. We could change that setting as well, if that is what is desired. On Axum: I don't think that this PR increases the degree to which the two libraries are intertwined - there is some private adaptor code added, but the Finally, it seems like you both have a preference for a re-written history here to break commits into more logical chunks - I'll take a stab at that as well, I had defaulted to preserving history to start, but am fine with either strategy. Overall, @djc, I would love specific comments on parts of this PR that are really separable - or the decision that we'd rather merge a series of PRs, some of which contain the pre-requistites to the upgrade to hyper-1.0, and the final one that contains the final upgrade. Given the scope of the API changes, I'm personally opposed to that latter solution, since sometimes solving for code which is compatible with pre-1.0 hyper and post-1.0 hyper adds additional complexity to the implementation, which we'd then have to clean up as well. |
|
@alexrudy FYI hyper-util v0.1.4 should contain the API you need above. |
|
@alexrudy as I can see, hyper-util v0.1.4 is released. Can you upgrade the PR? |
Trailers are now polled as a frame in the body - so we have to handle them at the same time that poll data. If trailers are encountered during client decode, they are stored and returned once poll_frame stops returning data frames. Since GRPC web frames are not aligned with underlying HTTP frames, we can’t simply return trailers once we find them - we have to use the presence of trailers to indicate the end of message frames.
The `poll_frame` method must produce the trailer frame along side the body data frames, so when we reach the end of the encoded data as the server, we encode the GRPC status as a trailing header and return that.
First pass at graceful shutdown
In-progress connections now prevent the server from returning until they are complete, ensuing that graceful termination can still happen.
See rust-lang/cargo#10788 for the full issue. When enabling a feature of a dependency, the syntax `tokio/rt` implicitly required a feature `tokio` to exist in 1.70 of cargo. By 1.77 this has been fixed. To work around this, we can use "weak feature dependencies" like `tokio?/rt` which enables the `rt` feature of tokio only if tokio is itself an enabled dependency. Also adding `dep:tokio` to the list of dependencies ensures that this is always true. When the MSRV is upgraded again, we can probably remove the weak `?` and the `dep:tokio` in these cases.
transport uses tokio::select!
aws-lc-crypto doesn't seem to compile by defualt on windows. Ideally, this should be a feature we toggle between.
It is only used by the TLS feature.
This requires CMake. Once again, a fast-follow should enable aws-lc-rs as a feature and the ability to switch between ring and aws-lc-rs
Found some places where links were not resolving correctly, this corrects them.
According to [hyperium#689](hyperium#689) streams should end after the first error. Therefore, when we find an error in the response step, discard the trailers so that the stream ends.
Server has to explicitly opt-in to upgrade support.
This provides proper support for `http2_only()` and `max_pending_accept_reset_streams()` settings.
Current Status: This is ready for CI & review!
This is a continuation of PR #1595 by @ikrivosheev.
TODO:
poll_frame. Still some work to do here about preserving data and trailers ifpoll_framereturns a frame type we aren't expecting.hyper-util)hyper-util, but also not too hard to do independently, followingaxum's example.)max_pending_accept_reset_streams(needs a release with PR #102 inhyper-util)http2_only(needs PR #111 inhyper-util.ConnectErrorsocket2dependency to support TCPSO_NODELAYandSO_KEEPALIVE