Upgrade to Hyper 1.0 & Axum 0.7

[alexrudy]

Current Status: This is ready for CI & review!

This is a continuation of PR #1595 by @ikrivosheev.

TODO:

• [X] update imports to use -util crates (done in #1595)
• [X] update AddrStream and AddrIncoming (done in #1595)
• [X] fix hyper-timeout connector (done in #1595)
• [X] use axum Request and Response in transport (via TowerToHyperService in Client)
• [X] change poll_trailers to use poll_frame. Still some work to do here about preserving data and trailers if poll_frame returns a frame type we aren't expecting.
• [X] Correctly handle data & trailer frames for server.
• [X] fix connect, Connect (no more hyper::client::service::Connect) (using hyper-util)
• [X] Get around "Implementation of From not general enough" compiler issue.
• [X] Correctly implement graceful shutdown (maybe based on hyper-util, but also not too hard to do independently, following axum's example.)
• [X] Pass through max_pending_accept_reset_streams (needs a release with PR #102 in hyper-util)
• [X] Correctly implement http2_only (needs PR #111 in hyper-util.
• [ ] Figure out what to do about hyper_util ConnectError
• [X] Add socket2 dependency to support TCP SO_NODELAY and SO_KEEPALIVE
• [X] Compatibility of other tonic-* crates (e.g. tonic-web)
• [X] Examples
• [X] Check over Docs
• [ ] Version Update

[alexrudy]

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 hyper and/or hyper-utils.

[karuna]

works perfectly after cargo update

[tisonkun]

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.

[tisonkun]

@alexrudy FYI https://github.com/hyperium/hyper-util/pull/102 is merged now.

[hubertshelley]

@alexrudy Example h2c failed. Terminal output:

called `Result::unwrap()` on an `Err` value: hyper::Error(User(ManualUpgrade))

[alexrudy]

@alexrudy Example h2c failed.

Thanks! https://github.com/hyperium/tonic/pull/1670/commits/5a579e63cadbe83c9dbe903472490c1d2397c904 contains the fix.

[djc]

@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:

• Since this is already such a large change, I'd like to avoid intertwining the changes that are necessary to upgrade hyper/axum with any other changes that can be done independently. For example, reformatting of Cargo.toml files, upgrading to a different nightly, reordering imports and other small changes like adding type annotations. To facilitate review I think all of those changes should either be reverted in this PR or submitted as part of separate PRs (with one commit or PR per logical change) where we can judge them properly -- the goal is to make this PR as small and focused as possible.
• I think it might make sense for now to just delete any of the examples that rely on warp, probably in a separate PR? While warp was one of the newer options when tonic got started, today it's much less popular than Axum -- and by virtue of doing this in a separate commit/PR it should be relatively straightforward to bring back those examples once warp upgrades to hyper/http 1.
• I would like to avoid further entangling of Axum and Tonic proper. Ideally this change should be neutral in how much of Axum is used in -- if that's hard for some reason let's discuss that separately? Especially for client-only users, we should be working in a direction where it's possible to avoid depending on Axum.

[alexrudy]

@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 hyper >= 1.

For instance:

• The change in the nightly compiler version from 2024-02-06 to 2024-04-16 is required to make Axum >= 0.7 compile on nightly (it otherwise requires #![feature(diagnostic_namespace)] to compile, and we can't change the nightly features requested by a dependency IIUC.
• There are type annotations added where type inference was no longer sufficient to allow code to compile - e.g. in the h2c/server.rs example, due to the move from calling hyper's server directly to calling TcpListener:bind(), type inference is no longer able to resolve the FromStr::Err type (which will get transformed to Box<dyn Error>, and in tls_rustls examples, types have to be added because rustls and rustls_pemfile have changed their public API types, which in turn are required upgrades (I think) for compatibility with hyper-rustls for hyper 1.0.

I will fix the cargo.toml formatting (this came from my IDE's auto formatter, and I should have caught that before).

For warp – I'm happy to delete those examples entirely, either in this PR or another one - deleting them is a pre-requisite to the CI tests passing.

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 tonic::transport::service::router part has always been built on the axum::Router class - are you suggesting that we should be implementing our own routing? That feels like a separate endeavor to me. Or was there somewhere else that this PR ends up intertwining the two further? From my perspective, I thought it was pretty neutral about how much of Axum is required by tonic.

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.

[tisonkun]

@alexrudy FYI hyper-util v0.1.4 should contain the API you need above.

[ikrivosheev]

@alexrudy as I can see, hyper-util v0.1.4 is released. Can you upgrade the PR?

https://crates.io/crates/hyper-util

[djc]

@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 hyper >= 1.

For instance:

* The change in the nightly compiler version from `2024-02-06` to `2024-04-16` is required to make Axum >= 0.7 compile on nightly (it otherwise requires `#![feature(diagnostic_namespace)]` to compile, and we can't change the nightly features requested by a dependency IIUC.

Makes sense. So can we bump this independently from the Axum upgrade or is this required to be "exactly" in sync? IMO this is one of these things that seems conceptually separate enough that it would be nice to deal with in a separate PR if possible.

* There are type annotations added where type inference was no longer sufficient to allow code to compile - e.g. in the h2c/server.rs example, due to the move from calling hyper's server directly to calling `TcpListener:bind()`, type inference is no longer able to resolve the `FromStr::Err` type (which will get transformed to `Box<dyn Error>`, and in `tls_rustls` examples, types have to be added because rustls and rustls_pemfile have changed their public API types, which in turn are required upgrades (I think) for compatibility with `hyper-rustls` for `hyper` 1.0.

That's fair! I'll point out detailed examples on my next, more detailed round of review.

I will fix the cargo.toml formatting (this came from my IDE's auto formatter, and I should have caught that before).

👍

For warp – I'm happy to delete those examples entirely, either in this PR or another one - deleting them is a pre-requisite to the CI tests passing.

Yes, let's take care of this in a separate PR.

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 tonic::transport::service::router part has always been built on the axum::Router class - are you suggesting that we should be implementing our own routing? That feels like a separate endeavor to me. Or was there somewhere else that this PR ends up intertwining the two further? From my perspective, I thought it was pretty neutral about how much of Axum is required by tonic.

Sounds good -- I'll take a look in more detail on my next read through.

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.

I think this repo has a squash merge strategy so doing one logical change per PR might be better. In other projects I help maintain I usually prefer to rebase and keep a clean history of atomic commits that deal with one conceptual change each -- exactly because in my opinion this facilitates review which is usually in shortest supply in open source projects. However, this is just fundamentally a large change (and I'm not advocating we split it up by doing piecemeal changes that try to paper over http 0.2/1.0 differences somehow).

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.

[alexrudy]

@djc (just about Axum & Nightly)

Makes sense. So can we bump this independently from the Axum upgrade or is this required to be "exactly" in sync? IMO this is one of these things that seems conceptually separate enough that it would be nice to deal with in a separate PR if possible.

We can bump the nightly version separately, thought it is then a one line change. I'll put up a PR for that - it is just a change which is (IMO) purely motivated by finding the minimum nightly where we can compile Axum.

[alexrudy]

@djc PR to remove warp examples

[alexrudy]

Nightly bump in a separate PR: https://github.com/hyperium/tonic/pull/1709

[djc]

The separate PRs have been merged -- will go through this again once it has been rebased.

[alexrudy]

I've both rebased this and tried to split the commits into roughly atomic parts - hopefully that makes it easier to review. Because of the large surface of api changes in hyper and http, I'm not sure if any of these remaining commits work as separate PRs since they all require the hyper and http upgrades.

[tottoto]

After a quick look, it appears that there are some unrelated changes included. Although I have not checked everything, here are some of them:

• The version style in Cargo.toml manifest such as 0.26 -> 0.26.0. If you want to propose these kind of changes, they should be issued as a separated pull request (but it is not the maintainer's preffered style: https://github.com/hyperium/tonic/pull/1589#discussion_r1608126077)
• There are some unnecessary version updating which seems not to be unnecessary: 2.0 -> 2.1.1, and so on.
• The kind of the changes like dropping logging feature of tokio-rustls is not related to this migration.
• The rebasing resolution seems to be broken, seeing the tls implementation.

For very large changes of this kind, it is necessary and important to reduce the scope of interest of the changes required in a single review as much as possible.

The impact of updating dependencies must also be considered in conjunction with the existing design. For example, dealing with tonic's Extensions, which is one of the effects of the breaking changes to the http crate, should not be handled simply, but should be handled by considering the necessity of the type in the first place. By the change of #1710 first, the responsibility for changing the update itself will be slightly reduced.

Or other example, since hyper no longer provides a Body type, I think it would be a good to have tonic's own Body type instead of BoxBody, like Axum does, in order to make the handling of the Body type in tonic more robust and ease to maintain, and it is possible to be addressed before updating hyper version. After that adopting it to hyper 1 can be addressed (Of course, whether this is actually possible requires separate consideration).

Basically, large changes like this should be broken up into smaller scope changes. For example, the scale of transport implementations is large, making it difficult to update them incrementally and therefore difficult to review.

The purpose of the following pull request is to improve both the user's convenience and the maintainability of this project itself, and I believe that these should be reflected first before migrating to hyper 1.

• 1572

• 1574

• 1630

[alexrudy]

Hi @tottoto,

Thanks for the comments!

This is a particularly painful kind of review for me, since I'm not sure that it consistently moves this PR towards being ready to merge, so I'll address your comments one-by-one.

The version style in Cargo.toml manifest such as 0.26 -> 0.26.0. If you want to propose these kind of changes, they should be issued as a separated pull request (but it is not the maintainer's preffered style: https://github.com/hyperium/tonic/pull/1589#discussion_r1608126077)

For the sake of getting this to merge, I'll revert that part of the change, despite the .0 being meaningful - I'm not sure what @djc meant by that comment originally?

There are some unnecessary version updating which seems not to be unnecessary: 2.0 -> 2.1.1, and so on.

I've reverted this particular version change. I'm not sure where the "and so on." comes from though - I have really tried to whittle down the dependencies. It is true that a lot of the tls-specific libraries get bumped, they are bumped to maintain compatibility with hyper-rustls (used in the examples) which requires the minor version bump.

The kind of the changes like dropping logging feature of tokio-rustls is not related to this migration.

Happy to put this back in. I'm not sure what function it is providing right now, since it isn't used by tonic.

The rebasing resolution seems to be broken, seeing the tls implementation.

I'm not sure what you mean by this - can you point to something specific for me?

Overall, I'm a little frustrated to keep coming back to this PR to find that folks are annoyed that it is so large - it is of course large, the scope of API changes between hyper 0.14 and hyper 1 is pretty huge. I also think that the desire to keep this PR as small as possible is pretty heavily in conflict with the desire to "consider dependencies" re. things like #1710, (which I'm fine with, but @djc mentioned there that they'd rather wait to land that until this lands).

The same applies to a Body type for tonic - that feels like a decent-sized API change which deserves its own PR, not something to be wedged into this giant morass.

Basically, large changes like this should be broken up into smaller scope changes. For example, the scale of transport implementations is large, making it difficult to update them incrementally and therefore difficult to review.

This really makes absolutely no sense to me - the transport changes have to land in-sync with the other changes because they are all required by the upgrade to hyper 1. Doing them separately would not make much sense. Alternatively, we could try to introduce some of the changes required for this PR in transport ahead of time, but that is rife with potential problems, since the API changes between hyper 0.14 and hyper 1 are so large, that writing the same code for hyper 0.14 is considerable additional effort which doesn't (at least to me, contributing on my free time) feel like it will pay off.

It also seems that you would prefer your list of PRs to be merged before this one:

The purpose of the following pull request is to improve both the user's convenience and the maintainability of this project itself, and I believe that these should be reflected first before migrating to hyper 1.

That's fine, but perhaps not a useful contribution here. In this thread, I think I've demonstrated that I'm very willing and responsive to rebasing this (massive) PR on top of additional changes to tonic. If you have a different view of how the project prioritizes reviews, maybe we could discuss that on Discord?

I think I feel a particular frustration here because you are the 3rd person to "take a quick look" and provide review comments which are difficult to take significant actions on. As well, although I'm probably mis-reading your comment, it comes off to me as pretty condescending to explain, yet again, that changes should be broken into small parts to make it easier on reviewers. I think that @djc and I have been having that conversation back-and-forth productively considering the tradeoffs up thread. Comments like "basically, large changes like this should be broken up into smaller scope changes" read like you are trying to educate someone with limited experience in the software world. I suspect you mean well, but in this case, I'd really encourage you to assume that the folks on the other end of your messages are knowledgable and competent developers, especially when they've clearly put a lot of time and effort into what they are building.

Again, thanks for the comments - I'm really looking forward to getting something like this merged, since it does also greatly impact my own user experience with tonic (since it prevents me from upgrading numerous other libraries which depend on hyper 1 now), which is a large part of my own motivation here.

[djc]

This is a particularly painful kind of review for me, since I'm not sure that it consistently moves this PR towards being ready to merge, so I'll address your comments one-by-one.

The version style in Cargo.toml manifest such as 0.26 -> 0.26.0. If you want to propose these kind of changes, they should be issued as a separated pull request (but it is not the maintainer's preffered style: #1589 (comment))

For the sake of getting this to merge, I'll revert that part of the change, despite the .0 being meaningful - I'm not sure what @djc meant by that comment originally?

I'm not sure exactly what part of the Cargo documentation leads you to believe that the trailing zero in 0.26.0 is meaningful, but I'm pretty sure it's not in practice. Both 0.26.0 and 0.26 mean the same thing, namely that the accepted version range is >= 0.26.0, < 0.27.0. This might be different for the case where the leading number is 0 because Cargo treats 0.x somewhat specially (in the sense that 0.1.x and 0.2.x are treated as semver-incompatible but 1.1.x and 1.2.x are treated as compatible).

There are some unnecessary version updating which seems not to be unnecessary: 2.0 -> 2.1.1, and so on.

I've reverted this particular version change. I'm not sure where the "and so on." comes from though - I have really tried to whittle down the dependencies. It is true that a lot of the tls-specific libraries get bumped, they are bumped to maintain compatibility with hyper-rustls (used in the examples) which requires the minor version bump.

The abundance of examples is unfortunate in my view -- IMO examples have diminishing returns as the number grows, and given that the maintenance efforts of examples is non-zero, increasing numbers of examples over time can have a net-negative value to the project (in particular for examples that drag in dependencies beyond what the libraries need anyway). So I think it would be good to have a discussion on what examples we ought to keep which might reduce the changes needed in this PR, but I also don't necessarily want to block this PR on those changes.

The kind of the changes like dropping logging feature of tokio-rustls is not related to this migration.

Happy to put this back in. I'm not sure what function it is providing right now, since it isn't used by tonic.

The logging feature is forwarded to rustls in the end, so it provides a feature that downstream users might belong on. I think defaulting to enabled makes sense here, but primarily I agree that this PR should not change whether we enable it or not because the change is unrelated to the change this PR seeks to make.

The rebasing resolution seems to be broken, seeing the tls implementation.

I'm not sure what you mean by this - can you point to something specific for me?

Overall, I'm a little frustrated to keep coming back to this PR to find that folks are annoyed that it is so large - it is of course large, the scope of API changes between hyper 0.14 and hyper 1 is pretty huge. I also think that the desire to keep this PR as small as possible is pretty heavily in conflict with the desire to "consider dependencies" re. things like #1710, (which I'm fine with, but @djc mentioned there that they'd rather wait to land that until this lands).

The same applies to a Body type for tonic - that feels like a decent-sized API change which deserves its own PR, not something to be wedged into this giant morass.

Agreed that these changes should go in separate PRs. I think the main question is whether they should be merged before this PR or other -- though at the moment I'm inclined to agree that it probably doesn't make sense to merge them before this PR.

Basically, large changes like this should be broken up into smaller scope changes. For example, the scale of transport implementations is large, making it difficult to update them incrementally and therefore difficult to review.

This really makes absolutely no sense to me - the transport changes have to land in-sync with the other changes because they are all required by the upgrade to hyper 1. Doing them separately would not make much sense. Alternatively, we could try to introduce some of the changes required for this PR in transport ahead of time, but that is rife with potential problems, since the API changes between hyper 0.14 and hyper 1 are so large, that writing the same code for hyper 0.14 is considerable additional effort which doesn't (at least to me, contributing on my free time) feel like it will pay off.

It also seems that you would prefer your list of PRs to be merged before this one:

The purpose of the following pull request is to improve both the user's convenience and the maintainability of this project itself, and I believe that these should be reflected first before migrating to hyper 1.

I think it's interesting to consider changes that isolate the impact of the specific dependencies used for the default transport implementation, but given that most downstream users are still likely to end up wanting to use the default transport implementation I feel like this is the more important PR.

Again, thanks for the comments - I'm really looking forward to getting something like this merged, since it does also greatly impact my own user experience with tonic (since it prevents me from upgrading numerous other libraries which depend on hyper 1 now), which is a large part of my own motivation here.

Thanks for your mature response to comments and sorry for being unable to spend more time on this PR -- tonic is just a small part of my job's dependency stack so while I want to keep things moving forward I have to be deliberate in how I spend my time on this. That said, as mentioned before this is a high priority for me and will definitely keep spending time on reviewing this.

[tottoto]

I think I feel a particular frustration here because you are the 3rd person to "take a quick look" and provide review comments which are difficult to take significant actions on.

I do not know what makes you frustrated, but I said a quick look to make it clear that I did not give a completely comprehensive review. And it is difficult to review for large changes that include changes that seem unrelated (I read this entire pull request, of course). Additionally, it does not matter that anybody be a 3rd person for reviewing. Recommended that you understand that receiving multiple similar points means that that point of view is considered important and needs to be addressed. I cannot do anything about difficulties to take significant action on makes you frustrated. Because your emotions are your own. Since it contains inherently difficult issues, it is inevitable that the content pointed out in reviews will be difficult. What I can say is that expressing frustration over unfavorable comments for you is not a good idea for OSS because it evokes fear that makes people hesitant to review. I believe that OSS contributors participate in the hope that OSS will become better.

When making large-scale changes (which is not a bad thing in itself; there are times when it is inherently inevitable, and I think this will be the case), we should judge the necessity of the changes more strictly than usual as possible to reduce the cognitive area required in the review. In this regard, your suggestions in this pull request are painful to review. I understand that you are tired of what is taking up your time, but I encourage you to realize that you are making unkind suggestions in reviews.

As well, although I'm probably mis-reading your comment, it comes off to me as pretty condescending to explain, yet again, that changes should be broken into small parts to make it easier on reviewers.

Yes. It is not the intended meaning. It seems that you are reading too deeply and are reading into things that are not written. It doesn't matter whether you're a seasoned developer or not, and I'm not talking about it. I am commenting on the code. You look be reluctant to basically make the changes smaller. In any case, at least I'm commenting on what I feel is necessary in the context, even if others have already pointed it out. In other words, at least from my point of view, it doesn't seem to be of sufficient quality in this respect. When we developers write code ourselves, we fully understand the scope of the code, so we tend to misunderstand some changes (including small refactoring) as trivial changes and underestimate the negative impact it will have on reviews. We tend to misunderstand, but the time it takes to write code has little to do with its quality.

The rebasing resolution seems to be broken, seeing the tls implementation.

I'm not sure what you mean by this - can you point to something specific for me?

Can you explain the need for the following changes to this pull request?

tonic/src/transport/service/tls.rs of https://github.com/hyperium/tonic/pull/1670/files#diff-f8078e6eaa6dfb7a56bc0ee9d2b6c345c231ebaa4c76d58e54faa95bf43945c4

[alexrudy]

@djc Thanks for the clarification about the version numbers - I see your point now. Its too bad that the current state of the Cargo.toml files for tonic is pretty inconsistent on this front.

The abundance of examples is unfortunate in my view -- IMO examples have diminishing returns as the number grows, and given that the maintenance efforts of examples is non-zero, increasing numbers of examples over time can have a net-negative value to the project (in particular for examples that drag in dependencies beyond what the libraries need anyway). So I think it would be good to have a discussion on what examples we ought to keep which might reduce the changes needed in this PR, but I also don't necessarily want to block this PR on those changes.

Happy to consider removing tls_rustls in a separate PR - that would reduce the changes required to the tls code elsewhere, although this issue is likely to re-emerge as users who want to leverage hyper-rustls start to upgrade.

[alexrudy]

@djc Super helpful comments - especially since I've been knee-deep in this PR, and some of it I inherited from the previous attempts. Thanks for taking the time. I've spent some time minimizing the formatting changes etc. that you commented on.

[LucioFranco]

I've gotten about halfway through the commits and will continue reviewing likely tomorrow depending on time. Left some feedback but mostly questions. Overall looks great. Thank you so much for breaking this up into commits, it has made it so much easier to review!

[alexrudy]

@LucioFranco I think I have addressed your comments!