Re: [go-nuts] Re: Some concerns with DialOpt

[Brad Fitzpatrick]

[+golang-dev]

Opinions wanted.  See below.

On Thu, Mar 28, 2013 at 4:49 AM, Anthony Martin <al...@pbrane.org> wrote:

I agree.

It would've been nice if the API conformed to the
code.google.com/p/go.net/proxy.Dialer interface.

Something like:

type DialOpts struct {
        Deadline     time.Time
        Network      string

        LocalAddress Addr
}

func (d *DialOpts) Dial(net, addr string) (Conn, error)

As it stands now, you have to add an extra closure
or top-level function to build a Dialer.

You, Taru, and others in this thread make some good points.

Using private interfaces and varargs is cute, but does prevent re-use in other packages.

To summarize for golang-dev:

-- net.Dial takes no options. this is bad.

-- net.DialTimeout only takes a timeout. this fixed a top complaint, but wasn't extensible enough. sorry.

-- there are many more options, for better or worse. (queue Rob's love for sockets)

-- in Go 1.0, we had no way to add new dial options.

-- in Go 1.1, we do want something that grows nicely in the future.

-- we don't want an interface like:

        net.DialStuff("tcp", "google.com:80", nil, nil, 0, nil, false, time.Time{}, nil)

   or even:

        net.DialOpt("google.com:80", nil)   // nil? gross. what is that?

-- whatever we do, we want to be able to add more options in Go 1.2 without changing

   too much.  Adding fields to a struct is fine. Adding new funcs to return DIalOptions is fine.

   Adding new types is starting to get gross. Adding new Dial funcs is off the table.

-- today in tip: net.DialOption is an interface with only private methods.

-- today in tip: all the functions to create options only return net.DialOption.

-- today in tip: it's hard for third-party packages to take a net.DialOption and do anything with it

   except pass it through unchanged. (for better or worse. for better: they can't forget to respect

   options. for worse: they can't respect options, nor test)

-- go.net contains a Dialer interface: http://godoc.org/code.google.com/p/go.net/proxy#Dialer

   type Dialer interface {

    // Dial connects to the given address via the proxy.

    Dial(network, addr string) (c net.Conn, err error)

    }

-- net/http.Transport contains an optional Dial func hook: http://golang.org/pkg/net/http/#Transport

        // Dial specifies the dial function for creating TCP

        // connections.

        // If Dial is nil, net.Dial is used.

        Dial func(net, addr string) (c net.Conn, err error)

-- we now have method values in tip (to be Go 1.1): http://tip.golang.org/ref/spec#Method_values

One of the earliest Dial option proposals was a struct of options.  That's still an option, and is slightly more attractive with method values now.

We could do:

package net

// Dialer specifies the parameters for making a network connection.

// The zero value is a valid Dialer. 

type Dialer struct {

      Timeout time.Duration

      Deadline time.Time

      LocalAddr net.Addr

      ....

      FastOption bool // (later, in Go 1.2)

}

func (d *Dialer) Dial func(net, addr string) (net.Conn, error)

And then you could say:

      dialer := &net.Dialer{

             LocalAddr: addr,

             Timeout: 15 * time.Second,

      }

      dialer.Dial("tcp", "google.com:80")

      // or:

      tr := &http.Transport{

           Dial: dialer.Dial,

      }

      // or

      wantsAnInterface(dialer)

That also seems acceptable.

Does anybody want to argue for or against either way?

We could still change it, but not for long.

[gary b]

Does anybody want to argue for or against either way?

I am in favor of the Dialer struct. 

I experimented with a DialOpt function in my Redis client. I defined a DialOption type and 

func NetOptions(opts ...net.DialOption) DialOption

for specifying options to net.DialOpt. I set this code aside for a couple of reasons:

- It's not possible to log or inspect the options from outside of the package that defines the options.

- I found that I was adding an unexported type, an exported function and a case in a switch statement for each option. This is a PITA compared to adding a field to a struct.

I considered defining a struct Options { ... } and adding a *Options argument to my Dial function, but that's not ideal because the Options struct does not follow the idiom in the net package.

The Dialer struct idiom addresses all of my concerns. An added bonus of the idiom is that my existing connection pool API can use the Dial method value with no changes to the connection pool.

One advantage of the varargs approach is that it's easy to distinguish between the absence of an option and the zero value for an option.

[Taru Karttunen]

On 01.04 16:24, Brad Fitzpatrick wrote:
> [+golang-dev]
>
> Opinions wanted. See below.

I would like to advocate having dialing with options not marked as
API stable and have it experimental for Go 1.1. Is this possible
from the API perspective? This would give time to find an API
that works for various concerns.

I do advocate a struct if this is not possible.

Additionally with the struct one could do e.g:

package tor

type Dialer struct {
net.Dialer
NoConnectionReuse bool
}

func (d *Dialer)Dial(network string, addr string) (net.Conn, error) {
...
}

Also various defaults can be supported:

var mydialerA = &Dialer{ ... }
var mydialerB = &Dialer{ ... }

tls:

package tls

type Dialer struct {
net.Dialer
Options
}

func (*Dialer)Dial ...

Marshalling & serializing? Of course. Low level options from syscall?

package net

type Dialer struct {
...
Syscall *syscall.NetOptions
}

etc

Marshalling&Unmarshalling? No problem. Extending things? No problem
either. Passing to e.g. net/http? No problem with method expressions.

- Taru Karttunen

[Brad Fitzpatrick]

On Mon, Apr 1, 2013 at 11:20 PM, Taru Karttunen <tar...@taruti.net> wrote:

On 01.04 16:24, Brad Fitzpatrick wrote:
> [+golang-dev]
>
> Opinions wanted.  See below.

I would like to advocate having dialing with options not marked as
API stable and have it experimental for Go 1.1. Is this possible
from the API perspective?

No.  We need to pick a color before Go 1.1.

 

I do advocate a struct if this is not possible.

Additionally with the struct one could do e.g:

package tor

type Dialer struct {
    net.Dialer
    NoConnectionReuse bool
}

That's another good argument for structs, thanks.

[gary b]

Additionally with the struct one could do e.g:

package tor

type Dialer struct {
    net.Dialer
    NoConnectionReuse bool
}

That's how I plan to structure my Redis dialer if the standard library converts to structs. 

[roger peppe]

I'm of the same opinion I was before - I think a struct is a nicer
and more idiomatic solution. If we can change it before 1.1, I'll
be happy.

[Julien Schmidt]

As long we find a solution based on an exported struct, I'm happy with that.

Passing the struct as an parameter to the dail function would be more consistent with the existing functions. But since the long term plan seems to be the removal of the two existing functions, I favor the dail method on the DailOpts struct, which feels very "go-ish".

[Brad Fitzpatrick]

Mailed https://codereview.appspot.com/8274043/

[roger peppe]

On 2 April 2013 20:25, Brad Fitzpatrick <brad...@golang.org> wrote:
> Mailed https://codereview.appspot.com/8274043/

wonderful, thanks!