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 <[email protected]>
remove `--ignore` option when pr(when hyperium/tonic#1670) is merged. Signed-off-by: Phoeniix Zhao <[email protected]>
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.
|
@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? |
hyper >= 1 provides its own I/O traits (Read & Write) instead of relying on the equivalent traits from `tokio`. Then, `hyper-util` provides adaptor structs to wrap `tokio` I/O objects and implement the hyper equivalents. Therefore, we update the appropriate bounds to use the hyper traits, and update the I/O objects so that they are wrapped in the tokio to hyper adaptor. Co-authored-by: Ivan Krivosheev <[email protected]> Co-authored-by: Allan Zhang <[email protected]>
Axum must be >= 0.7 to support hyper >= 1 Doing this also involves changing the Body type used. Since hyper >= 1 does not provide a generic body type, Axum and tonic both use `BoxBody` to provide a pointer to a Body. This changes the trait bounds required for methods which accept additional Serivces to be run alongside the primary GRPC service, since those will be routed with Axum, and therefore must accept a BoxBody. Co-authored-by: Ivan Krivosheev <[email protected]> Co-authored-by: Allan Zhang <[email protected]>
Hyper >= 1 no longer includes automatic http2/http1 combined connections, and so we must swtich to the `http2::Builder` type (this is okay, we set http2_only(true) anyhow). As well, hyper >= 1 is generic over executors and does not directly depend on tokio. Since http2 connections can be multiplexed, they require some additional background task to handle sending and receiving requests. Additionally, these background tasks do not natively implement `tower::Service` since hyper >= 1 does not depend on `tower`. Therefore, we re-implement the `SendRequest` task as a tower::Service, so that it can be used within `Connection`, which expects to operate on a tower::Service to serve connections. Co-authored-by: Ivan Krivosheev <[email protected]> Co-authored-by: Allan Zhang <[email protected]>
`hyper::Client` has been moved to `hyper_util::legacy::Client` in version 1. Co-authored-by: Ivan Krivosheev <[email protected]> Co-authored-by: Allan Zhang <[email protected]>
hyper::Error no longer provides information about Connect errors, especially since hyper_util now contains the connection implementation, it does not provide a separate error type. Instead, we create an internal Error type which is used in our own connectors, and then checked when figuring out what the gRPC status should be.
hyper >= 1 has deprecated all of `hyper::server`, including `AddrStream` Co-authored-by: Ivan Krivosheev <[email protected]> Co-authored-by: Allan Zhang <[email protected]> Replace hyper::server::Accept hyper::server is deprectaed. Instead, we implement our own TCP-incoming based on the now removed hyper::server::Accept. In order to set `TCP_KEEPALIVE` we require the socket2 crate, since this option is not exposed in the standard library’s API. The implementaiton is inspired by that of hyper v0.14
hyper::Server is deprecated, with no current common replacement. Instead of implementing (or using tonic’s new) full server in here, we write a simple accept loop, which is sufficient to demonstrate the functionality of h2c.
hyper-rustls requires version 0.27.0 to support hyper >= 1, bringing a few other tls bumps along. Importantly, we add the “ring” and “tls12” features to use ring as the crypto backend, consistent with previous versions of tonic. A future version of tonic might support selecting backends via features. Co-authored-by: Ivan Krivosheev <[email protected]>
We aren't sure if multiple trailers should even be legal, but if we get multiple trailers in an HTTP body stream, we'll combine them all, to preserve their data. Alternatively we'd have to pick the first or last trailers, and that might lose information.
Example used `empty_body()`, which is now fully qualified as `tonic::body::empty_body()` to make clear that this is a tonic helper method for creating an empty BoxBody.
Ideally, a body should only return a single trailer frame. If multiple trailers are returned, merge them together.
Comment mentions why we choose “Unavailable” for connection errors
This also requires vendoring it in the rustls example, which doesn’t use a server type. Making the type crate-private means we can delete some unused methods.
|
@LucioFranco I think I have addressed your comments! |
There was a problem hiding this comment.
LGTM! Thanks to everyone for following up and getting this work completed. This is probably the biggest refactor that tonic has seen and I appreciate the patience. This will get released soon, I am going to collect a few other breaking changes and then ship this all together. There is a v0.11.x branch that will keep support for hyper v0.14.
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