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.

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

feat(transport): port router to axum #830

Merged
merged 5 commits into from Feb 18, 2022
Merged

Conversation

davidpdrsn
Copy link
Member

This ports tonic's previous routing setup to axum. This is mostly just a proposal. I think it lead to some decent wins but might not make that much difference in practice. Its a breaking change so we have to wait until 0.7 if we decide to go this way.

  • The router type is no longer generic and conceptually simpler.
  • Should resolve any compile time issues people might be having due to regression in Rust 1.56.
  • Performance might be slightly better if users are routing between a lot of services. This is due to axum's router using matchit rather than the nested services.
  • I also removed Never since axum requires services to use Infallible. Can easily convert between them but if we're making breaking changes we might as well remove Never.
  • add_service now requires service's to be infallible. This shouldn't cause any breakage since generated services were already infallible, the trait bounds just didn't require it.

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big big big fan of this! One question really...and I think we should update the readme hyping this up a bit :)

tonic-build/src/client.rs Outdated Show resolved Hide resolved
tonic/src/body.rs Outdated Show resolved Hide resolved
tonic/src/transport/server/mod.rs Show resolved Hide resolved
tonic/src/transport/server/mod.rs Show resolved Hide resolved
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit: 🔥 I think this is one of the most exciting PRs we've had in a while, super hyped for this!

@LucioFranco
Copy link
Member

We also probably want to consider doing a blog post for this, since it shows how the ecosystem can glue together and that tonic and axum are not really competitors in a sense.

@davidpdrsn
Copy link
Member Author

We also probably want to consider doing a blog post for this, since it shows how the ecosystem can glue together and that tonic and axum are not really competitors in a sense.

Thats a great idea! I'll do that when we ship the next major version of tonic 😊

@davidpdrsn davidpdrsn added this to the 0.7 milestone Nov 15, 2021
where
S: Service<Request<Body>, Response = Response<BoxBody>>
S: Service<Request<Body>, Response = Response<BoxBody>, Error = Infallible>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do this in this PR or another but we should maybe makeRequest<impl Body> instead of hard coding it to hyper?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I think we should explore it separately though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants