code review 7365049: net: add DialOpt, the extensible Dial w/ options dialer

net: add DialOpt, the extensible Dial w/ options dialer

Add DialOpt. So we have:

func Dial(net, addr string) (Conn, error)
func DialTimeout(net, addr string, timeout time.Duration) (Conn, error)
func DialOpt(addr string, opts ...DialOption) (Conn, error)

DialTimeout (and Dial) are regrettable in retrospect. Maybe
in a future Go we'll be back down to one Dial, with DialOpt
becoming Dial.

DialOpt looks like:

c, err := net.DialOpt("google.com:80")  // tcp is default
c, err := net.DialOpt("google.com:80", net.Timeout(30 * time.Second))
c, err := net.DialOpt("google.com:80", net.TCPFastOpen())
c, err := net.DialOpt("google.com:80", net.LocalAddr(..))
c, err := net.DialOpt("google.com:53", net.Network("udp6"))

And then: (clustered in godoc)

type DialOption interface { /* private only */ }
  func Deadline(time.Time) DialOption
  func LocalAddr(Addr) DialOption
  func Network(string) DialOption
  func TCPFastOpen() DialOption
  func Timeout(time.Duration) DialOption

I'm pretty confident we could add Happy Eyeballs to this too.

Fixes issue 3097
Update issue 3610
Update issue 4842

[bradfitz, 2013-02-22 01:10:17]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2013-02-22 01:12:59]

Note: it's tempting to just modify the net.Dial signature, but that's an
API change.

On Thu, Feb 21, 2013 at 5:10 PM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> net: add DialOpt, the extensible Dial w/ options dialer
>
> Proposal.
>
> Add DialOpt. So we have:
>
> func Dial(net, addr string) (Conn, error)
> func DialTimeout(net, addr string, timeout time.Duration) (Conn, error)
> func DialOpt(addr string, opts ...DialOption) (Conn, error)
>
> Dials like:
>
> c, err := net.DialOpt("google.com:80")  // tcp is default
> c, err := net.DialOpt("google.com:80", net.Timeout(30 * time.Second))
> c, err := net.DialOpt("google.com:80", net.TCPFastOpen())
> c, err := net.DialOpt("google.com:80", net.LocalAddr(..))
> c, err := net.DialOpt("google.com:53", net.Network("udp6"))
>
> And then: (clustered in godoc)
>
> type DialOption interface { /* private only */ }
>   func Deadline(time.Time) DialOption
>   func LocalAddr(...) DialOption
>   func Network(string) DialOption
>   func TCPFastOption() DialOption
>   func Timeout(time.Duration) DialOption
>
> I'm pretty confident we could add Happy Eyeballs to this too.
>
> Update issue 3097
> Update issue 3610
> Update issue 4842
>
> Please review this at
https://codereview.appspot.**com/7365049/<https://codereview.appspot.com/7365...
>
> Affected files:
>   M src/pkg/net/dial.go
>
>
> Index: src/pkg/net/dial.go
> ==============================**==============================**=======
> --- a/src/pkg/net/dial.go
> +++ b/src/pkg/net/dial.go
> @@ -6,6 +6,65 @@
>
>  import "time"
>
> +// A DialOption modifies a DialOpt call.
> +type DialOption interface {
> +       dialOption()
> +}
> +
> +var (
> +       // TCP is a dial option to dial with TCP (over IPv4 or IPv6).
> +       TCP = Network("tcp")
> +
> +       // UDP is a dial option to dial with UDP (over IPv4 or IPv6).
> +       UDP = Network("udp")
> +)
> +
> +// Network returns a DialOption to dial using the given network.
> +//
> +// Known networks are "tcp", "tcp4" (IPv4-only), "tcp6" (IPv6-only),
> +// "udp", "udp4" (IPv4-only), "udp6" (IPv6-only), "ip", "ip4"
> +// (IPv4-only), "ip6" (IPv6-only), "unix", "unixgram" and
> +// "unixpacket".
> +//
> +// For IP networks, net must be "ip", "ip4" or "ip6" followed
> +// by a colon and a protocol number or name, such as
> +// "ipv4:1" or "ip6:ospf".
> +func Network(net string) DialOption {
> +       return dialNetwork(net)
> +}
> +
> +type dialNetwork string
> +
> +func (dialNetwork) dialOption() {}
> +
> +// Deadline returns a DialOption to fail a dial that doesn't
> +// complete before t.
> +func Deadline(t time.Time) DialOption {
> +       return dialDeadline(t)
> +}
> +
> +// Timeout returns a DialOption to fail a dial that doesn't
> +// complete before the provided duration.
> +func Timeout(d time.Duration) DialOption {
> +       return dialDeadline(time.Now().Add(d)**)
> +}
> +
> +type dialDeadline time.Time
> +
> +func (dialDeadline) dialOption() {}
> +
> +type tcpFastOpen struct{}
> +
> +func (tcpFastOpen) dialOption() {}
> +
> +// TCPFastTimeout returns an option to use TCP Fast Open (TFO) when
> +// doing this dial. It is only valid for use with TCP connections.
> +// Data sent over a TFO connection may be processed by the peer
> +// multiple times, so should be used with caution.
> +func TCPFastTimeout() DialOption {
> +       return tcpFastOpen{}
> +}
> +
>  func parseNetwork(net string) (afnet string, proto int, err error) {
>         i := last(net, ':')
>         if i < 0 { // no colon
> @@ -75,11 +134,38 @@
>  //     Dial("ip6:ospf", "::1")
>  //
>  func Dial(net, addr string) (Conn, error) {
> -       ra, err := resolveAddr("dial", net, addr, noDeadline)
> +       return DialOpt(addr, dialNetwork(net))
> +}
> +
> +func netFromOptions(opts []DialOption) string {
> +       for _, opt := range opts {
> +               if p, ok := opt.(dialNetwork); ok {
> +                       return string(p)
> +               }
> +       }
> +       return "tcp"
> +}
> +
> +func deadlineFromOptions(opts []DialOption) time.Time {
> +       for _, opt := range opts {
> +               if d, ok := opt.(dialDeadline); ok {
> +                       return time.Time(d)
> +               }
> +       }
> +       return noDeadline
> +}
> +
> +// DialOpt dials addr using the provided options.
> +// If no options are provided, DialOpt(addr) is equivalent
> +// to Dial("tcp", addr). See Dial for the syntax of addr.
> +func DialOpt(addr string, opts ...DialOption) (Conn, error) {
> +       net := netFromOptions(opts)
> +       deadline := deadlineFromOptions(opts)
> +       ra, err := resolveAddr("dial", net, addr, deadline)
>         if err != nil {
>                 return nil, err
>         }
> -       return dial(net, addr, ra, noDeadline)
> +       return dial(net, addr, ra, deadline)
>  }
>
>  func dial(net, addr string, ra Addr, deadline time.Time) (c Conn, err
> error) {
>
>
>

[r, 2013-02-22 01:13:38]

LGTM from an API perspective, but a networking maven should weigh in too

https://codereview.appspot.com/7365049/diff/5001/src/pkg/net/dial.go
File src/pkg/net/dial.go (right):

https://codereview.appspot.com/7365049/diff/5001/src/pkg/net/dial.go#newcode47
src/pkg/net/dial.go:47: // complete before the provided duration.
s/before/within/ ?

[dave_cheney.net, 2013-02-22 01:16:28]

Strawman: Were you thinking of Dial(net, addr string, opts ...DialOption) ?

On Fri, Feb 22, 2013 at 12:12 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Note: it's tempting to just modify the net.Dial signature, but that's an API
> change.
>
>
> On Thu, Feb 21, 2013 at 5:10 PM, <bradfitz@golang.org> wrote:
>>
>> Reviewers: golang-dev_googlegroups.com,
>>
>> Message:
>> Hello golang-dev@googlegroups.com,
>>
>> I'd like you to review this change to
>> https://go.googlecode.com/hg/
>>
>>
>> Description:
>> net: add DialOpt, the extensible Dial w/ options dialer
>>
>> Proposal.
>>
>> Add DialOpt. So we have:
>>
>> func Dial(net, addr string) (Conn, error)
>> func DialTimeout(net, addr string, timeout time.Duration) (Conn, error)
>> func DialOpt(addr string, opts ...DialOption) (Conn, error)
>>
>> Dials like:
>>
>> c, err := net.DialOpt("google.com:80")  // tcp is default
>> c, err := net.DialOpt("google.com:80", net.Timeout(30 * time.Second))
>> c, err := net.DialOpt("google.com:80", net.TCPFastOpen())
>> c, err := net.DialOpt("google.com:80", net.LocalAddr(..))
>> c, err := net.DialOpt("google.com:53", net.Network("udp6"))
>>
>> And then: (clustered in godoc)
>>
>> type DialOption interface { /* private only */ }
>>   func Deadline(time.Time) DialOption
>>   func LocalAddr(...) DialOption
>>   func Network(string) DialOption
>>   func TCPFastOption() DialOption
>>   func Timeout(time.Duration) DialOption
>>
>> I'm pretty confident we could add Happy Eyeballs to this too.
>>
>> Update issue 3097
>> Update issue 3610
>> Update issue 4842
>>
>> Please review this at https://codereview.appspot.com/7365049/
>>
>> Affected files:
>>   M src/pkg/net/dial.go
>>
>>
>> Index: src/pkg/net/dial.go
>> ===================================================================
>> --- a/src/pkg/net/dial.go
>> +++ b/src/pkg/net/dial.go
>> @@ -6,6 +6,65 @@
>>
>>  import "time"
>>
>> +// A DialOption modifies a DialOpt call.
>> +type DialOption interface {
>> +       dialOption()
>> +}
>> +
>> +var (
>> +       // TCP is a dial option to dial with TCP (over IPv4 or IPv6).
>> +       TCP = Network("tcp")
>> +
>> +       // UDP is a dial option to dial with UDP (over IPv4 or IPv6).
>> +       UDP = Network("udp")
>> +)
>> +
>> +// Network returns a DialOption to dial using the given network.
>> +//
>> +// Known networks are "tcp", "tcp4" (IPv4-only), "tcp6" (IPv6-only),
>> +// "udp", "udp4" (IPv4-only), "udp6" (IPv6-only), "ip", "ip4"
>> +// (IPv4-only), "ip6" (IPv6-only), "unix", "unixgram" and
>> +// "unixpacket".
>> +//
>> +// For IP networks, net must be "ip", "ip4" or "ip6" followed
>> +// by a colon and a protocol number or name, such as
>> +// "ipv4:1" or "ip6:ospf".
>> +func Network(net string) DialOption {
>> +       return dialNetwork(net)
>> +}
>> +
>> +type dialNetwork string
>> +
>> +func (dialNetwork) dialOption() {}
>> +
>> +// Deadline returns a DialOption to fail a dial that doesn't
>> +// complete before t.
>> +func Deadline(t time.Time) DialOption {
>> +       return dialDeadline(t)
>> +}
>> +
>> +// Timeout returns a DialOption to fail a dial that doesn't
>> +// complete before the provided duration.
>> +func Timeout(d time.Duration) DialOption {
>> +       return dialDeadline(time.Now().Add(d))
>> +}
>> +
>> +type dialDeadline time.Time
>> +
>> +func (dialDeadline) dialOption() {}
>> +
>> +type tcpFastOpen struct{}
>> +
>> +func (tcpFastOpen) dialOption() {}
>> +
>> +// TCPFastTimeout returns an option to use TCP Fast Open (TFO) when
>> +// doing this dial. It is only valid for use with TCP connections.
>> +// Data sent over a TFO connection may be processed by the peer
>> +// multiple times, so should be used with caution.
>> +func TCPFastTimeout() DialOption {
>> +       return tcpFastOpen{}
>> +}
>> +
>>  func parseNetwork(net string) (afnet string, proto int, err error) {
>>         i := last(net, ':')
>>         if i < 0 { // no colon
>> @@ -75,11 +134,38 @@
>>  //     Dial("ip6:ospf", "::1")
>>  //
>>  func Dial(net, addr string) (Conn, error) {
>> -       ra, err := resolveAddr("dial", net, addr, noDeadline)
>> +       return DialOpt(addr, dialNetwork(net))
>> +}
>> +
>> +func netFromOptions(opts []DialOption) string {
>> +       for _, opt := range opts {
>> +               if p, ok := opt.(dialNetwork); ok {
>> +                       return string(p)
>> +               }
>> +       }
>> +       return "tcp"
>> +}
>> +
>> +func deadlineFromOptions(opts []DialOption) time.Time {
>> +       for _, opt := range opts {
>> +               if d, ok := opt.(dialDeadline); ok {
>> +                       return time.Time(d)
>> +               }
>> +       }
>> +       return noDeadline
>> +}
>> +
>> +// DialOpt dials addr using the provided options.
>> +// If no options are provided, DialOpt(addr) is equivalent
>> +// to Dial("tcp", addr). See Dial for the syntax of addr.
>> +func DialOpt(addr string, opts ...DialOption) (Conn, error) {
>> +       net := netFromOptions(opts)
>> +       deadline := deadlineFromOptions(opts)
>> +       ra, err := resolveAddr("dial", net, addr, deadline)
>>         if err != nil {
>>                 return nil, err
>>         }
>> -       return dial(net, addr, ra, noDeadline)
>> +       return dial(net, addr, ra, deadline)
>>  }
>>
>>  func dial(net, addr string, ra Addr, deadline time.Time) (c Conn, err
>> error) {
>>
>>
>
> --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[bradfitz, 2013-02-22 01:20:57]

Yes. That changes the type net.Dial, so it's a no-go.

package myprogram

var dial = net.Dial  // var for tests to change

func program() {
    c, err := dial("tcp", "example.com")
    ....
}

package myprogram // test

var dialResc = make(chan struct { net.Conn; error })
func init() {
   dial = fakeDialForTest  // BOOM in Dave's proposal
}

func fakeDialForTest(net, addr string) (net.Conn, error) {
      dialRes := <-dialResc
      return dialRes.Conn, dialRes.error
}

On Thu, Feb 21, 2013 at 5:16 PM, Dave Cheney <dave@cheney.net> wrote:

> Strawman: Were you thinking of Dial(net, addr string, opts ...DialOption) ?
>
> On Fri, Feb 22, 2013 at 12:12 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > Note: it's tempting to just modify the net.Dial signature, but that's an
> API
> > change.
> >
> >
> > On Thu, Feb 21, 2013 at 5:10 PM, <bradfitz@golang.org> wrote:
> >>
> >> Reviewers: golang-dev_googlegroups.com,
> >>
> >> Message:
> >> Hello golang-dev@googlegroups.com,
> >>
> >> I'd like you to review this change to
> >> https://go.googlecode.com/hg/
> >>
> >>
> >> Description:
> >> net: add DialOpt, the extensible Dial w/ options dialer
> >>
> >> Proposal.
> >>
> >> Add DialOpt. So we have:
> >>
> >> func Dial(net, addr string) (Conn, error)
> >> func DialTimeout(net, addr string, timeout time.Duration) (Conn, error)
> >> func DialOpt(addr string, opts ...DialOption) (Conn, error)
> >>
> >> Dials like:
> >>
> >> c, err := net.DialOpt("google.com:80")  // tcp is default
> >> c, err := net.DialOpt("google.com:80", net.Timeout(30 * time.Second))
> >> c, err := net.DialOpt("google.com:80", net.TCPFastOpen())
> >> c, err := net.DialOpt("google.com:80", net.LocalAddr(..))
> >> c, err := net.DialOpt("google.com:53", net.Network("udp6"))
> >>
> >> And then: (clustered in godoc)
> >>
> >> type DialOption interface { /* private only */ }
> >>   func Deadline(time.Time) DialOption
> >>   func LocalAddr(...) DialOption
> >>   func Network(string) DialOption
> >>   func TCPFastOption() DialOption
> >>   func Timeout(time.Duration) DialOption
> >>
> >> I'm pretty confident we could add Happy Eyeballs to this too.
> >>
> >> Update issue 3097
> >> Update issue 3610
> >> Update issue 4842
> >>
> >> Please review this at https://codereview.appspot.com/7365049/
> >>
> >> Affected files:
> >>   M src/pkg/net/dial.go
> >>
> >>
> >> Index: src/pkg/net/dial.go
> >> ===================================================================
> >> --- a/src/pkg/net/dial.go
> >> +++ b/src/pkg/net/dial.go
> >> @@ -6,6 +6,65 @@
> >>
> >>  import "time"
> >>
> >> +// A DialOption modifies a DialOpt call.
> >> +type DialOption interface {
> >> +       dialOption()
> >> +}
> >> +
> >> +var (
> >> +       // TCP is a dial option to dial with TCP (over IPv4 or IPv6).
> >> +       TCP = Network("tcp")
> >> +
> >> +       // UDP is a dial option to dial with UDP (over IPv4 or IPv6).
> >> +       UDP = Network("udp")
> >> +)
> >> +
> >> +// Network returns a DialOption to dial using the given network.
> >> +//
> >> +// Known networks are "tcp", "tcp4" (IPv4-only), "tcp6" (IPv6-only),
> >> +// "udp", "udp4" (IPv4-only), "udp6" (IPv6-only), "ip", "ip4"
> >> +// (IPv4-only), "ip6" (IPv6-only), "unix", "unixgram" and
> >> +// "unixpacket".
> >> +//
> >> +// For IP networks, net must be "ip", "ip4" or "ip6" followed
> >> +// by a colon and a protocol number or name, such as
> >> +// "ipv4:1" or "ip6:ospf".
> >> +func Network(net string) DialOption {
> >> +       return dialNetwork(net)
> >> +}
> >> +
> >> +type dialNetwork string
> >> +
> >> +func (dialNetwork) dialOption() {}
> >> +
> >> +// Deadline returns a DialOption to fail a dial that doesn't
> >> +// complete before t.
> >> +func Deadline(t time.Time) DialOption {
> >> +       return dialDeadline(t)
> >> +}
> >> +
> >> +// Timeout returns a DialOption to fail a dial that doesn't
> >> +// complete before the provided duration.
> >> +func Timeout(d time.Duration) DialOption {
> >> +       return dialDeadline(time.Now().Add(d))
> >> +}
> >> +
> >> +type dialDeadline time.Time
> >> +
> >> +func (dialDeadline) dialOption() {}
> >> +
> >> +type tcpFastOpen struct{}
> >> +
> >> +func (tcpFastOpen) dialOption() {}
> >> +
> >> +// TCPFastTimeout returns an option to use TCP Fast Open (TFO) when
> >> +// doing this dial. It is only valid for use with TCP connections.
> >> +// Data sent over a TFO connection may be processed by the peer
> >> +// multiple times, so should be used with caution.
> >> +func TCPFastTimeout() DialOption {
> >> +       return tcpFastOpen{}
> >> +}
> >> +
> >>  func parseNetwork(net string) (afnet string, proto int, err error) {
> >>         i := last(net, ':')
> >>         if i < 0 { // no colon
> >> @@ -75,11 +134,38 @@
> >>  //     Dial("ip6:ospf", "::1")
> >>  //
> >>  func Dial(net, addr string) (Conn, error) {
> >> -       ra, err := resolveAddr("dial", net, addr, noDeadline)
> >> +       return DialOpt(addr, dialNetwork(net))
> >> +}
> >> +
> >> +func netFromOptions(opts []DialOption) string {
> >> +       for _, opt := range opts {
> >> +               if p, ok := opt.(dialNetwork); ok {
> >> +                       return string(p)
> >> +               }
> >> +       }
> >> +       return "tcp"
> >> +}
> >> +
> >> +func deadlineFromOptions(opts []DialOption) time.Time {
> >> +       for _, opt := range opts {
> >> +               if d, ok := opt.(dialDeadline); ok {
> >> +                       return time.Time(d)
> >> +               }
> >> +       }
> >> +       return noDeadline
> >> +}
> >> +
> >> +// DialOpt dials addr using the provided options.
> >> +// If no options are provided, DialOpt(addr) is equivalent
> >> +// to Dial("tcp", addr). See Dial for the syntax of addr.
> >> +func DialOpt(addr string, opts ...DialOption) (Conn, error) {
> >> +       net := netFromOptions(opts)
> >> +       deadline := deadlineFromOptions(opts)
> >> +       ra, err := resolveAddr("dial", net, addr, deadline)
> >>         if err != nil {
> >>                 return nil, err
> >>         }
> >> -       return dial(net, addr, ra, noDeadline)
> >> +       return dial(net, addr, ra, deadline)
> >>  }
> >>
> >>  func dial(net, addr string, ra Addr, deadline time.Time) (c Conn, err
> >> error) {
> >>
> >>
> >
> > --
> >
> > ---
> > You received this message because you are subscribed to the Google Groups
> > "golang-dev" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to golang-dev+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/groups/opt_out.
> >
> >
>

[minux1, 2013-02-22 06:47:45]

i have an (probably bad) idea:
introduce this means a whole lot of networking packages also need
DialOpt like interface which has a snow-ball effect imo.

could we add the option somehow to the address or network string?
and provide various constructor like this:
func SetTCPKeepAlive(addr string) string

it will incur allocations at every dial, so perhaps it should be added
to the net part.

[bradfitz, 2013-02-22 07:04:55]

On Thu, Feb 21, 2013 at 10:47 PM, <minux.ma@gmail.com> wrote:

> i have an (probably bad) idea:
> introduce this means a whole lot of networking packages also need
> DialOpt like interface


Why is that?


> could we add the option somehow to the address or network string?
> and provide various constructor like this:
> func SetTCPKeepAlive(addr string) string
>

Sure, in theory, but we could also in theory stop using all static types
and use interface{} for everything.


> it will incur allocations at every dial, so perhaps it should be added
> to the net part.
>

Compared to dialing and what happens with that connection afterwards, I'm
not concerned about a tiny allocation (which the compiler could in-theory
eliminate, if it doesn't largely already).

[minux1, 2013-02-22 08:38:38]

On Fri, Feb 22, 2013 at 3:04 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> On Thu, Feb 21, 2013 at 10:47 PM, <minux.ma@gmail.com> wrote:
>
>> i have an (probably bad) idea:
>
> introduce this means a whole lot of networking packages also need
>> DialOpt like interface
>
>
> Why is that?
>
For example, do we need DialOpt in crypto/tls?
of course, we can say that people want this flexibility should use
tls.Client directly.

>
>
>> could we add the option somehow to the address or network string?
>> and provide various constructor like this:
>> func SetTCPKeepAlive(addr string) string
>>
>
> Sure, in theory, but we could also in theory stop using all static types
> and use interface{} for everything.
>
yes, this idea is not type safe. I was just worried about introducing
DialOpt like interface
to other networking related packages.
But, perhaps it's better to get it all done with a single DialOpt than with
a lot of Dail* functions, so this new API is ok.

We should have used a similar solution before adding DialTimeout, though.
we probably could eliminate DialTimeout in Go 2. ;-)

[minux1, 2013-02-22 15:50:07]

I wonder if we could define DialOption as an interface?

type DialOption interface {
    // handle could set any options (ioctls) on the connection or even
replace the incoming conn.
    // This isn't a good name for the method, but you get the idea.
    // we could make the interface private for now.
    handle(conn net.Conn, addr string) (net.Conn, err)
}

then in DialOpt, we just queue a Dial("tcp", addr) in the front of options
and then invoke the
handle method of all provided options in order (passing the net.Conn
through the chain).

When we think the interface is mature enough, we can make it public, so
that people could
make their application-specific DialOption (of course, they are on their
own if they do this).
(and yes, when 3rd-party packages are doing this, they must dig the fd out
like go.net/ipv4)

perhaps there will be too much overhead for DialOpt(addr, NetOption("udp"))?
(but of course we can optimize this case)

[bradfitz, 2013-02-22 18:50:54]

On Fri, Feb 22, 2013 at 7:49 AM, minux <minux.ma@gmail.com> wrote:

> I wonder if we could define DialOption as an interface?


It already _is_ an interface.  See the CL.  I'm confused.


> type DialOption interface {
>     // handle could set any options (ioctls) on the connection or even
> replace the incoming conn.
>     // This isn't a good name for the method, but you get the idea.
>     // we could make the interface private for now.
>     handle(conn net.Conn, addr string) (net.Conn, err)
> }
>
then in DialOpt, we just queue a Dial("tcp", addr) in the front of options
> and then invoke the
> handle method of all provided options in order (passing the net.Conn
> through the chain).
>

That doesn't work for many of the options, or not without a lot of
refactoring.  We currently need to complete dialing before making a
net.Conn.  But with dial timeouts, your proposal would need to have a
net.Conn pre-dial.  Also, TCP_FASTOPEN needs to be set before you dial.
 How does that work in your proposal?

Your proposal seems way more complicated, but has the exact same API:  an
interface with one private method.


> When we think the interface is mature enough, we can make it public, so
> that people could
> make their application-specific DialOption (of course, they are on their
> own if they do this).
>  (and yes, when 3rd-party packages are doing this, they must dig the fd
> out like go.net/ipv4)
>

I would rather not ever export this.  Seems like too much chaos to support,
especially when the implementors of said interface would just be using
reflect to reach into internals and mess with stuff using unsafe.  No
thanks.


> perhaps there will be too much overhead for DialOpt(addr,
> NetOption("udp"))?
> (but of course we can optimize this case)
>

That's what I figured:  a) dialing/etc is more expensive anyway, 2) we can
fix the compiler if anybody ever cares enough.

[bradfitz, 2013-02-22 18:53:09]

On Fri, Feb 22, 2013 at 12:38 AM, minux <minux.ma@gmail.com> wrote:

>
> On Fri, Feb 22, 2013 at 3:04 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:
>
>> On Thu, Feb 21, 2013 at 10:47 PM, <minux.ma@gmail.com> wrote:
>>
>>> i have an (probably bad) idea:
>>
>> introduce this means a whole lot of networking packages also need
>>> DialOpt like interface
>>
>>
>> Why is that?
>>
> For example, do we need DialOpt in crypto/tls?
>

No, because there is already http://golang.org/pkg/crypto/tls/#Client which
takes a net.Conn.

Also, tls.Dial already takes a pointer to a struct, so we could add
[]net.DialOption to *Config if we really wanted to. (I don't.)

[rsc, 2013-02-22 18:57:58]

Can we just make DialOpt a struct? Making it an interface means adding tons
more API to package net (one top-level func per option), and package net
already has way too much API.

Russ

[bradfitz, 2013-02-22 19:21:20]

On Fri, Feb 22, 2013 at 10:57 AM, Russ Cox <rsc@golang.org> wrote:

> Can we just make DialOpt a struct?
>

That's like the original DialPlan proposal. With a Dial method on the Opt
struct? I could write that up as a separate CL so we can compare the two.

[rsc, 2013-02-22 20:23:16]

My main objection to this CL is just the proliferation of API in package
net. The godoc output is already overwhelming. But perhaps this doesn't
make it much worse. I do like the general idea. If we were starting from
scratch, there's a good argument for making this net.Dial.

Russ

[bradfitz, 2013-02-22 20:28:33]

On Fri, Feb 22, 2013 at 12:23 PM, Russ Cox <rsc@golang.org> wrote:

> My main objection to this CL is just the proliferation of API in package
> net. The godoc output is already overwhelming. But perhaps this doesn't
> make it much worse. I do like the general idea. If we were starting from
> scratch, there's a good argument for making this net.Dial.
>

My hunch is that the struct way adds just as much API (in terms of lines to
api/go11.txt) but it's just lighter in godoc HTML visually.  I could've
probably made this much lighter in HTML by using vars instead of funcs in a
few places, but I was actually optimizing *for* more HTML, so things would
group under the DialOption type.  (I was watching my local godoc as I
developed it)

Maybe we're both wrong for designing APIs for how they will render in
today's godoc.

I don't know.

[rsc, 2013-02-27 16:42:45]

LGTM

[bradfitz, 2013-02-27 16:46:33]

On Wed, Feb 27, 2013 at 8:42 AM, Russ Cox <rsc@golang.org> wrote:

> LGTM
>

I'm confused.  I haven't even prototyped the struct version yet.  I thought
you/we wanted to compare the two approaches?

Is that a LGTM or SGTM or ... ?

[rsc, 2013-02-27 18:00:12]

I think the DialOpt is nicer than the struct. As long as the new API all
appears under type DialOption I think it's fine.

[r, 2013-02-27 18:29:53]

https://codereview.appspot.com/7365049/diff/5001/src/pkg/net/dial.go
File src/pkg/net/dial.go (right):

https://codereview.appspot.com/7365049/diff/5001/src/pkg/net/dial.go#newcode47
src/pkg/net/dial.go:47: // complete before the provided duration.
either that or 'before the end of the duration'.

[bradfitz, 2013-02-27 19:51:28]

Hello golang-dev@googlegroups.com, r@golang.org, dave@cheney.net,
minux.ma@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-02-27 19:52:22]

This is ready for review + submit now.

Updated CL description, added LocalAddr, and hid the TCPFastOpen option
with a TODO until it's implemented in a future CL.


On Wed, Feb 27, 2013 at 11:51 AM, <bradfitz@golang.org> wrote:

> Hello golang-dev@googlegroups.com, r@golang.org, dave@cheney.net,
> minux.ma@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
https://codereview.appspot.**com/7365049/<https://codereview.appspot.com/7365...
>

[r, 2013-02-27 19:53:31]

LGTM

[bradfitz, 2013-02-27 19:59:41]

*** Submitted as https://code.google.com/p/go/source/detail?r=54731addb542 ***

net: add DialOpt, the extensible Dial w/ options dialer

Add DialOpt. So we have:

func Dial(net, addr string) (Conn, error)
func DialTimeout(net, addr string, timeout time.Duration) (Conn, error)
func DialOpt(addr string, opts ...DialOption) (Conn, error)

DialTimeout (and Dial) are regrettable in retrospect. Maybe
in a future Go we'll be back down to one Dial, with DialOpt
becoming Dial.

DialOpt looks like:

c, err := net.DialOpt("google.com:80")  // tcp is default
c, err := net.DialOpt("google.com:80", net.Timeout(30 * time.Second))
c, err := net.DialOpt("google.com:80", net.TCPFastOpen())
c, err := net.DialOpt("google.com:80", net.LocalAddr(..))
c, err := net.DialOpt("google.com:53", net.Network("udp6"))

And then: (clustered in godoc)

type DialOption interface { /* private only */ }
  func Deadline(time.Time) DialOption
  func LocalAddr(Addr) DialOption
  func Network(string) DialOption
  func TCPFastOpen() DialOption
  func Timeout(time.Duration) DialOption

I'm pretty confident we could add Happy Eyeballs to this too.

Fixes issue 3097
Update issue 3610
Update issue 4842

R=golang-dev, r, dave, minux.ma, rsc
CC=golang-dev
https://codereview.appspot.com/7365049

https://codereview.appspot.com/7365049/diff/5001/src/pkg/net/dial.go
File src/pkg/net/dial.go (right):

https://codereview.appspot.com/7365049/diff/5001/src/pkg/net/dial.go#newcode47
src/pkg/net/dial.go:47: // complete before the provided duration.
On 2013/02/22 01:13:38, r wrote:
> s/before/within/ ?

Done.

[julienschmidt, 2013-02-28 02:36:44]

That may well be a little late, but I just saw the commit and honestly, I think
this is way to complex. Why not something as simple as this:

type DialOptions struct {
	LocalAdress net.Addr
	TCPFastOpen bool
	Timeout     time.Duration
}

func Dial(net, addr string) (net.Conn, error) {
	return DialOpts(net, addr, &DialOptions{})
}

func DialOpts(net, add string, opts *DialOptions) (net.Conn, error) { // Not
sure if opts should be a pointer
	ra, err := resolveAddr("dial", net, addr, opts.Timeout) // time.Duration here!
	if err != nil {
		return nil, err
	}
	// merge func dial(..) in here
}


Example Usage:
	net.DialOpts("tcp", "google.com:80", &net.DialOptions{
		TCPFastOpen: true,
		Timeout:     30 * time.Second,
	})

It is more consistent with the existing functions (and doesn't make them
obsolete), doesn't extend the API so much and if I'm not completely wrong, this
also saves a few byte memory and a few CPU cycles.

Would a struct like this lead to problems with the API contract when it should
be extended?


And one other thing:
>func Timeout(d time.Duration) DialOption {
> 	return dialDeadline(time.Now().Add(d))
>}

Doesn't this result in unexpected behavior if you create the DialOption some
time before calling Dial?
The Deadline might even be passed before Dial is called despite the fact that
you set a timeout of x seconds.
That is why I use time.Duration instead time.Time in the draft above. The actual
deadline should be created somewhere internally right before the actual dial
starts.