grpc.Dial spins forever if target address is bad about grpc-go

[dsymonds]

The net package (http://golang.org/pkg/net/) has a bunch of specific error types (e.g. AddrError or InvalidAddrError that can tell you such details. Or you can do an interface check (with net.Error) to see if the error has a Temporary() bool method that will tell you if it's a permanent failure.

[iamqizhao]

yup, it is on my radar. Plan to add a dialOption to enforce a limit.

[iamqizhao]

or have a context.Context for transport so that you can set deadline/timeout or issue cancellation.

[dsymonds]

Let's not just sprinkle options for things that we can simply do the right thing. A dial failure can fail in permanent/temporary ways. There's no point in doing any retries if the address is completely malformed, since it'll never get a connection. But a grpc.DialTimeout would be okay.

A context isn't a great choice for dialing. Contexts are meant to be request-scoped, and dialing should rarely be in the scope of a request. The OS primitives usually aren't cancellable either.

[iamqizhao]

So you proposed adding another API grpc.DialTimeout? sgtm.

Regarding connection-scoped context, it was originally from bcmills on a previous code review. I actually think it is probably a good idea (but maybe overkill). For OS primitive, it is not special for connection-scoped context. It is the issue for request-scoped context too (e.g., a socket write stuck in kernel when the corresponding context is canceled/timeout).

[dsymonds]

No, I suggest grpc.Dial should have a ...grpc.DialOption variadic argument, and have grpc.DialTimeout to return such an option.

[iamqizhao]

On Thu, Feb 26, 2015 at 5:20 PM, David Symonds [email protected]
wrote:

No, I suggest grpc.Dial should have a ...grpc.DialOption variadic
argument, and have grpc.DialTimeout to return such an option.

This is exactly what the current impl is (
https://github.com/grpc/grpc-go/blob/master/clientconn.go#L83) and my
original proposal then ...

—
Reply to this email directly or view it on GitHub
#76 (comment).

[dsymonds]

Your original proposal said "a configurable limit on retries", which I interpreted to mean a limit on the number of retries, which I think is a bad option. A limit on the total time trying to dial would be fine though. Even better would be to (also) detect the types of errors, and for the known-permanent failures (e.g. malformed address) have grpc.Dial fail immediately.

[iamqizhao]

On Thu, Feb 26, 2015 at 5:53 PM, David Symonds [email protected]
wrote:

Your original proposal said "a configurable limit on retries", which I
interpreted to mean a limit on the number of retries, which I think is a
bad option. A limit on the total time trying to dial would be fine though.

I see. Sorry for confusion. Actually I was debating which one (timeout vs #
retries) is better at that time.

Even better would be to (also) detect the types of errors, and for the
known-permanent failures (e.g. malformed address) have grpc.Dial fail
immediately.

Yup. Is it possible to get some hint from the error returned by net.Dial
(e.g., DNS resolving failure)?

—
Reply to this email directly or view it on GitHub
#76 (comment).

[iamqizhao]

On Thu, Feb 26, 2015 at 6:39 PM, David Symonds [email protected]
wrote:

The net package (http://golang.org/pkg/net/) has a bunch of specific
error types (e.g. AddrError or InvalidAddrError that can tell you such
details. Or you can do an interface check (with net.Error) to see if the
error has a Temporary() bool method that will tell you if it's a
permanent failure.

Nice, this is the thing I missed. Thanks!

—
Reply to this email directly or view it on GitHub
#76 (comment).

[iamqizhao]

fixed in #93