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

Why is DialContext not blocking by default? #1953

Closed
nhooyr opened this issue Mar 29, 2018 · 5 comments · Fixed by #1970
Closed

Why is DialContext not blocking by default? #1953

nhooyr opened this issue Mar 29, 2018 · 5 comments · Fixed by #1970
Assignees
Labels
P1 Type: Documentation Documentation or examples

Comments

@nhooyr
Copy link

nhooyr commented Mar 29, 2018

Please answer these questions before submitting your issue.

What version of gRPC are you using?

1.9.2

What version of Go are you using (go version)?

go version go1.10 darwin/amd64

What operating system (Linux, Windows, …) and version?

macOS 10.13.3

What did you do?

If possible, provide a recipe for reproducing the error.

package main

import (
	"context"
	"log"
	"time"

	"google.golang.org/grpc"
)

func main() {
	ctx, cancel := context.WithTimeout(context.Background(), time.Second)
	defer cancel()
	_, err := grpc.DialContext(ctx, "example.com",
		grpc.WithInsecure(),
	)
	log.Print(err)
}

What did you expect to see?

An error

What did you see instead?

nil

This is because DialContext does not block by default. It should as otherwise, it doesn't really make sense to pass in a Context. If someone is passing in a Context to a Dial, its pretty much guaranteed that they want it to block as they want the Context to apply to the dial. Its extremely counter intuitive to run the dial in the background and immediately return.

@nhooyr
Copy link
Author

nhooyr commented Mar 29, 2018

Ah, its from #858

I think this behaviour should be changed or clearly documented on grpc.DialContext. I can whip up the PR if this sounds good.

@nhooyr
Copy link
Author

nhooyr commented Mar 29, 2018

Or maybe we should just deprecate grpc.DialContext and recommend users who want to timeout the dial use the WithDialer option and use a timeout inside of that.

@dfawley
Copy link
Member

dfawley commented Mar 29, 2018

Or maybe we should just deprecate grpc.DialContext and recommend users who want to
timeout the dial use the WithDialer option and use a timeout inside of that.

This has different connotations. A grpc.ClientConn is not a single connection, it is a connection pool. If you apply a timeout in your dialer, that applies to that particular connection attempt. If you apply a timeout to Dial, that applies across all of the initial connection attempts.

In #1786, I have proposed deprecating Dial/DialContext and replacing them with a NewClient[Conn] instead, which would make that distinction more apparent, and eliminate blocking client creation altogether. (Users would still be able to use the connectivity state API to achieve the same result, if desired.)

It would probably be appropriate to mention the non-blocking default behavior in the docstring.

@nhooyr
Copy link
Author

nhooyr commented Mar 29, 2018

That all sounds great to me.

@nhooyr nhooyr closed this as completed Mar 29, 2018
@nhooyr nhooyr reopened this Mar 29, 2018
@nhooyr
Copy link
Author

nhooyr commented Mar 29, 2018

I'll keep this open to track the clarification of the docs.

@dfawley dfawley added the Type: Documentation Documentation or examples label Mar 29, 2018
@menghanl menghanl added the P1 label Apr 12, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Oct 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P1 Type: Documentation Documentation or examples
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants