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.

Sign up for GitHub

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

Jump to bottom

0.3.0 Client::connect always returns Ok even if the sever URL does not exist. #403

Closed
DominicWrege opened this issue Jul 16, 2020 · 12 comments · Fixed by #413
Closed

0.3.0 Client::connect always returns Ok even if the sever URL does not exist. #403

DominicWrege opened this issue Jul 16, 2020 · 12 comments · Fixed by #413
Assignees
@LucioFranco
Labels
A-tonic C-bug Category: Something isn't working

Comments

@DominicWrege
Copy link

Bug Report

Version 0.3.0

Platform

tonic-client-bug v0.1.0 (/tmp/tonic-0.3.0-client-bug)
└── tonic v0.3.0
└── tonic-build v0.2.0

Crates

tonic = { version = "0.3.0", features = ["tls"] }
prost = "0.6.1"
tokio = { version = "0.2.21", features = ["full"] }
futures = "0.3.5"
async-stream = "0.2.1"

Description

I excepted that the client returns an Err if no connection to the sever could establish.
Example Repository: https://github.com/DominicWrege/tonic-0.3.0-client-bug
@DominicWrege DominicWrege changed the title 0.3.0 Client::connect always returns Ok even if the sever URL does not exists. 0.3.0 Client::connect always returns Ok even if the sever URL does not exist. Jul 16, 2020
@LucioFranco
Copy link
Member

I wonder if it is related to #392, will have to take a look later this week. Thanks for opening the issue :)

@LucioFranco LucioFranco added C-bug Category: Something isn't working A-tonic labels Jul 20, 2020
@LucioFranco LucioFranco self-assigned this Jul 20, 2020
@DominicWrege
Copy link
Author

Thanks for your response. So the basically connect is the new lazy_connect ? Or did I misunderstood something?
I my opinion it should be very explicated if the connection should be established lazy or not.
My problem is that I want to check if the server is available by simply trying if a can get a connection.

@LucioFranco
Copy link
Member

@DominicWrege no I mean't that that change may have introduced unexpected behavior.

@Rulliam
Copy link

Rulliam commented Jul 27, 2020 •
edited
Loading

I experienced the same behavior, thank you for taking the time to fix this <3

LucioFranco added a commit that referenced this issue Jul 27, 2020
@LucioFranco
fix(transport): Return connection error on Channel::connect
1909775 
Before this fix, if the connect phase of the transport failed before
ever establishing a connection, we would never return the error until
the first call to send a request. This PR changes that behavior to only
forward the error to the call method if we have ever made a connection
before. If we have never established a connection before then
`Reconnect` will return an error on the call to `poll_ready`.

Fixes #403
@LucioFranco LucioFranco mentioned this issue Jul 27, 2020
Merged
@LucioFranco
Copy link
Member

@DominicWrege this should fix it #413, I wrote a small description in the PR for why it was happening.

LucioFranco added a commit that referenced this issue Jul 27, 2020
@LucioFranco
fix(transport): Return connection error on Channel::connect (#413)
2ea17b2 
Before this fix, if the connect phase of the transport failed before
ever establishing a connection, we would never return the error until
the first call to send a request. This PR changes that behavior to only
forward the error to the call method if we have ever made a connection
before. If we have never established a connection before then
`Reconnect` will return an error on the call to `poll_ready`.

Fixes #403
@DominicWrege
Copy link
Author

thx

@Rulliam
Copy link

Rulliam commented Jul 28, 2020

Thank you ! works like a charm !

@faern
Copy link
Contributor

faern commented Aug 20, 2020

Will there be a 0.3.1 (or similar) release with this fix soon? We are currently holding off upgrading from 0.2 due to this issue.

@LucioFranco
Copy link
Member

@faern should be out in the next 10min #436

@LucioFranco
Copy link
Member

Ok, 0.3.1 is out.

@faern
Copy link
Contributor

faern commented Aug 20, 2020

Awesome! Thank you for this incredible response time!

@LucioFranco
Copy link
Member

We like our low p99s 😉

@boxdot boxdot mentioned this issue Sep 22, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@LucioFranco LucioFranco

Labels
A-tonic C-bug Category: Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(transport): Return connection error on Channel::connect hyperium/tonic
4 participants
@faern @LucioFranco @DominicWrege @Rulliam