-
Notifications
You must be signed in to change notification settings - Fork 1k
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
fix(tonic): change connect_lazy
to be infallible
#712
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but as this is a breaking change we cannot merge yet. We literally shipped a breaking release yesterday so this just missed the cut 😜
Added to 0.6 milestone but will probably be some time before that gets shipped.
CHANGELOG.md
Outdated
@@ -38,6 +38,7 @@ This release includes a new crate `tonic-web` which is versioned at `0.1` and su | |||
* **transport:** Add a tls-webpki-roots feature to add trust roots from webpki-roots ([#660](https://github.com/hyperium/tonic/issues/660)) ([32173dc](https://github.com/hyperium/tonic/commit/32173dc7f6521bad8f26b055b6a86d807348f151)) | |||
* **transport:** add connect timeout to `Endpoint` ([#662](https://github.com/hyperium/tonic/issues/662)) ([2b60a00](https://github.com/hyperium/tonic/commit/2b60a00614c5c4260ce0acaaa599da89bebfd267)) | |||
* **transport:** provide generic access to connect info ([#647](https://github.com/hyperium/tonic/issues/647)) ([e5e3118](https://github.com/hyperium/tonic/commit/e5e311853bff347355722bc829d40f54e8954aee)) | |||
* **transport:** Refactor Endpoint::connect_lazy method to return Channel directly instead of Result ([#392](https://github.com/hyperium/tonic/issues/708)) ([3b45565](https://github.com/3b45565b178a7298622c6b35bc6705b309beb410 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't update the changelog in each PR so can just remove this line.
Thanks! :) |
connect_lazy
to be infallible
connect_lazy
to be infallibleconnect_lazy
to be infallible
Fixes #708
Motivation
Most of the object creation and error validation is already done in
Endpoint::new
and as suchEndpoint::connect_lazy
should not be returning any errors since the connection is evaluated on-demand and cannot fail when building a channel. Meanwhile the API gives the wrong impression that the call may fail (does it ping the service beforehand? who knows)Solution
Use
Channel
directly instead of wrapping it inOk(Channel)