support unix domain socket

[iamqizhao]

No description provided.

[dsymonds]

This feels too specific. It doesn't help this support any other address types either. Why don't we simply accept a dialer option that takes complete responsibility for turning the address into a net.Conn?

[iamqizhao]

On Fri, Apr 17, 2015 at 3:44 PM, David Symonds notifications@github.com
wrote:

In clientconn.go
#169 (comment):

@@ -95,6 +96,14 @@ func WithTimeout(d time.Duration) DialOption {
}
}

+// WithNetwork returns a DialOption that specifies the network on which
+// the connection will be established.
+func WithNetwork(network string) DialOption {

This feels too specific. It doesn't help this support any other address
types either. Why don't we simply accept a dialer option that takes
complete responsibility for turning the address into a net.Conn?

I do not get it. Can you elaborate? What is "a dialer option"? Are you
proposing to have our own struct to take complete responsibility for
turning the addr to a net.Conn?

—
Reply to this email directly or view it on GitHub
https://github.com/grpc/grpc-go/pull/169/files#r28635771.

[dsymonds]

I mean, instead of a WithNetwork function, have something like

func WithDialer(f func(addr string) (net.Conn, error)) DialOption

and then use that function in place of net.Dial (or as the dialer passed to tls.Dial).

[iamqizhao]

On Fri, Apr 17, 2015 at 4:02 PM, David Symonds notifications@github.com
wrote:

I mean, instead of a WithNetwork function, have something like

func WithDialer(f func(addr string) (net.Conn, error)) DialOption

and then use that function in place of net.Dial (or as the dialer passed
to tls.Dial).

This is definitely more generic. My concern is that it pushes a bit too
much work to users -- e.g., users have to write dialing code for uds.

When you said "It doesn't help this support any other address types
either", what example did you have in mind? I assumed the addr string
matches the named network when users call WithNetwork.

—

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

[dsymonds]

If you're wanting to dial UDS, it's all of one line to do the dial (invoke net.DialUnix), so it's relatively little work for that use case to wrap that in a closure.

There's lots of other dialing requirements, like Google-internal things, or dialing on App Engine (e.g. you need to use the socket API there), or wanting to monitor/control/log outbound connection information, and so on.

[iamqizhao]

The problem/difficulty to take this approach is that it has to merge with
dial timeout option.

https://github.com/grpc/grpc-go/blob/master/transport/http2_client.go#L118

and

https://github.com/grpc/grpc-go/blob/master/transport/http2_client.go#L113

If we merge them, again it seems it complicates the task if a user only
wants to set a dial timeout -- he has to implement a fully functional
dialing function...

On Fri, Apr 17, 2015 at 4:23 PM, David Symonds notifications@github.com
wrote:

If you're wanting to dial UDS, it's all of one line to do the dial (invoke
net.DialUnix), so it's relatively little work for that use case to wrap
that in a closure.

There's lots of other dialing requirements, like Google-internal things,
or dialing on App Engine (e.g. you need to use the socket API there), or
wanting to monitor/control/log outbound connection information, and so on.

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

[dsymonds]

That the code is already a bit weird in trying to handle timeouts is unfortunate, and should itself probably be fixed. I flagged that previously, but didn't have the time to pursue it.

I wasn't suggesting getting rid of WithTimeout, only WithNetwork.

A timeout is really the only option to the dialer that makes sense across almost every dialer, so it seems reasonable to make the signature be func WithDialer(f func(addr string, timeout time.Duration) (net.Conn, error)) DialOption.

[iamqizhao]

On Fri, Apr 17, 2015 at 11:29 PM, David Symonds notifications@github.com
wrote:

That the code is already a bit weird in trying to handle timeouts is
unfortunate, and should itself probably be fixed. I flagged that
previously, but didn't have the time to pursue it.

I wasn't suggesting getting rid of WithTimeout, only WithNetwork.

A timeout is really the only option to the dialer that makes sense across
almost every dialer, so it seems reasonable to make the signature be func
WithDialer(f func(addr string, timeout time.Duration) (net.Conn, error))
DialOption.

This still has some issues. For plain tcp, WithTimeout and the proposed
WithDialer cannot coexist (the timeout set by WithTimeout will be
discarded). There are more problems for tls connection. If users use
WithDialer to provide a closure to generate net.Conn (plain tcp) from addr
string, tlsCreds needs to use tls.Client to create a TLS client side
connection from the plain tcp connection and call tls.Conn.Handshake to
finish the tls handshaking. This is a bit troublesome comparing to the
current code. In additional, the timeout specified by WithDialer only takes
care of the duration for establish a plain tcp connection and does not
cover the tls handshaking latency.

I had some discussion with java and c grpc teams. They plan to do / is
doing the approach which puts network type into the addr string (e.g.,
unix:///mysock.unix and tcp://host:port). Probably we can do the same thing
here and remove WithNetwork. The special treatment of dialing on AppEngine
& monitor/log/control can be achieved in some other way.

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

[dsymonds]

My latest suggested WithDialer should be passed a timeout, which would be set from WithTimeout. So they work together fine. It's then up to the dialer to use that timeout appropriately.

grpc.Dial can use the tls package directly, and tell it to use a dialer, which would be a closure invoking whatever is passed in via WithDialer. The current code already does something vaguely along these lines; my proposal only slightly generalises it.

[iamqizhao]

On Mon, Apr 20, 2015 at 9:43 PM, David Symonds notifications@github.com
wrote:

My latest suggested WithDialer should be passed a timeout, which would be
set from WithTimeout. So they work together fine. It's then up to the
dialer to use that timeout appropriately.

grpc.Dial can use the tls package directly, and tell it to use a dialer,
which would be a closure invoking whatever is passed in via WithDialer. The
current code already does something vaguely along these lines; my proposal
only slightly generalises it.

tls.DialWithDialer takes a *net.Dialer to support timeout. net.Dialer is a
struct. How can we use the user specified closure here? The only way I see
is to user tls.Client and tls.Conn's Handshake(). What do I miss here?

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

[dsymonds]

You don't have to use tls.DialWithDialer. It's a helper if you have a net.Dialer. You can use tls.Client and tls.Handshake over an existing net.Conn. That's only a few lines of code.

[iamqizhao]

On Mon, Apr 20, 2015 at 10:22 PM, David Symonds notifications@github.com
wrote:

You don't have to use tls.DialWithDialer. It's a helper if you have a
net.Dialer. You can use tls.Client and tls.Handshake over an existing
net.Conn. That's only a few lines of code.

That is exactly what I said in the previous emails... Then as I mentioned
before, it is somewhat trouble some and has some code duplication (from
tls.DialWithDialer) to support timeout for Handshake() -- you need to do
some subtraction to get the timeout value for Handshake and mimic what
tls.DialWithDialer does for Handshake timeout ... It seems we basically
duplicate the code of DialWithDialer here (into 2 pieces: dialer and
handshake) ... In general, this is fine with me and I can take on this
tomorrow. It would be nice if you can point me to an example for app engine
dialing so that I can be convinced this is a real demand and make sure the
new one will be well fit for it. :) Thanks.

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

[dsymonds]

Sorry, yes, I didn't read your previous email. Yes, your plan seems fine. You can see the App Engine dialing reference doc at https://cloud.google.com/appengine/docs/go/sockets/reference. A dialer to use that would need to curry in the appengine.Context arg, but otherwise it is trivial to do if you can support the WithDialer option.