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

client: fix FailOnNonTempDialError and add a test for it #2276

Merged
merged 1 commit into from
Aug 27, 2018

Conversation

virtuald
Copy link
Contributor

@thelinuxfoundation
Copy link

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

Regards,
CLA GitHub bot

@virtuald
Copy link
Contributor Author

Added CLA.

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for putting this fix together. One minor comment on the test, otherwise 👍 👍


func TestDialContextFailFast(t *testing.T) {
defer leakcheck.Check(t)
ctx := context.Background()
Copy link
Member

Choose a reason for hiding this comment

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

Please make this timeout eventually so the test won't hang forever if it breaks. E.g.

ctx := context.WithTimeout(context.Background(), 10*time.Second)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why don't any of the other tests do that then?

Copy link
Member

Choose a reason for hiding this comment

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

🤷‍♂️ 😞

I'm fixing the ones I see when I run into them.

@virtuald
Copy link
Contributor Author

Also, thinking about it, should a note be added to the documentation that FailOnNonTempDialError is only useful if WithBlock is also used? It's not immediately clear to me that it does anything if not using WithBlock...

@dfawley
Copy link
Member

dfawley commented Aug 27, 2018

Also, thinking about it, should a note be added to the documentation that FailOnNonTempDialError is only useful if WithBlock is also used? It's not immediately clear to me that it does anything if not using WithBlock...

That would be great, thanks.

The whole idea of a blocking Dial is a bit unusual for the gRPC model, where a ClientConn is actually a long-lived pool of connections that may be unavailable at times. The initial connection attempt is only interesting in that if it fails, it's possible your configuration was incorrect, vs. transient network/unavailability problems.
Note that in all other languages, gRPC does not attempt to connect until the first RPC is dispatched, and the initial connection is not treated specially.

@virtuald
Copy link
Contributor Author

Added documentation note also.

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Thanks!

@dfawley dfawley changed the title Fix FailOnNonTempDialError and adds a test for it client: fix FailOnNonTempDialError and add a test for it Aug 27, 2018
@dfawley dfawley merged commit 91c7ef8 into grpc:master Aug 27, 2018
@virtuald virtuald deleted the fix-fail-non-temp branch August 27, 2018 18:36
@dfawley dfawley added this to the 1.15 Release milestone Aug 30, 2018
dfawley pushed a commit to dfawley/grpc-go that referenced this pull request Sep 12, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Feb 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants