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

fix(transport): reconnect lazy connections after first failure #458

Merged
merged 2 commits into from
Sep 23, 2020

Conversation

alce
Copy link
Collaborator

@alce alce commented Sep 19, 2020

Channels created with lazy connections never try to reconnect if the first connection attempt fails. This is because Reconnect returns Poll::Ready(Err) on poll_ready and the service is considered dead.

This change passes a flag to Reconnect to signal if the connection is intended to be lazy, in which case reconnect returns the error on the next call. A new Connection::lazy constructor is provided to avoid threading booleans around.

fixes #452

Channels created with lazy connections never try to reconnect if the
first connection attempt fails. This is because `Reconnect` returns
`Poll::Ready(Err)` on poll_ready and the service is considered dead.

This change passes a flag to Reconnect to signal if the connection
is intended to be lazy, in which case reconnect returns the error on
the next call.

fixes hyperium#452
tests/integration_tests/tests/connection.rs Outdated Show resolved Hide resolved
tonic/src/transport/service/reconnect.rs Outdated Show resolved Hide resolved
tonic/src/transport/service/reconnect.rs Outdated Show resolved Hide resolved
@alce alce requested a review from LucioFranco September 22, 2020 21:56
@alce alce merged commit e9910d1 into hyperium:master Sep 23, 2020
@alce alce deleted the reconnect branch September 23, 2020 14:35
@dfreese
Copy link
Contributor

dfreese commented Mar 9, 2021

It looks like this change, in combination with #392 leaves the Result<Channel, Error> return type, but no longer is able to fail. Would there be interest in just having this function return Channel instead?

@LucioFranco
Copy link
Member

@dfreese yeah, I think we need to revisit some of the APIs but likely won't change anytime soon since its a breaking change and I would like to limit that until we have a proper design for larger changes.

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