Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(223)

Issue 7365049: code review 7365049: net: add DialOpt, the extensible Dial w/ options dialer (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 1 month ago by bradfitz
Modified:
11 years, 1 month ago
Reviewers:
julienschmidt
CC:
golang-dev, r, dfc, minux1, rsc
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : diff -r fbb1de9bc379 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r fbb1de9bc379 https://go.googlecode.com/hg/ #

Total comments: 3

Patch Set 4 : diff -r bae71e582148 https://go.googlecode.com/hg/ #

Patch Set 5 : diff -r e0b23d56fd6a https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+134 lines, -11 lines) Patch
M src/pkg/net/dial.go View 1 2 3 3 chunks +133 lines, -10 lines 0 comments Download
M src/pkg/net/fd_unix.go View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 24
bradfitz
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
11 years, 1 month ago (2013-02-22 01:10:17 UTC) #1
bradfitz
Note: it's tempting to just modify the net.Dial signature, but that's an API change. On ...
11 years, 1 month ago (2013-02-22 01:12:59 UTC) #2
r
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 ...
11 years, 1 month ago (2013-02-22 01:13:38 UTC) #3
dfc
Strawman: Were you thinking of Dial(net, addr string, opts ...DialOption) ? On Fri, Feb 22, ...
11 years, 1 month ago (2013-02-22 01:16:28 UTC) #4
bradfitz
Yes. That changes the type net.Dial, so it's a no-go. package myprogram var dial = ...
11 years, 1 month ago (2013-02-22 01:20:57 UTC) #5
minux1
i have an (probably bad) idea: introduce this means a whole lot of networking packages ...
11 years, 1 month ago (2013-02-22 06:47:45 UTC) #6
bradfitz
On Thu, Feb 21, 2013 at 10:47 PM, <minux.ma@gmail.com> wrote: > i have an (probably ...
11 years, 1 month ago (2013-02-22 07:04:55 UTC) #7
minux1
On Fri, Feb 22, 2013 at 3:04 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > On Thu, Feb ...
11 years, 1 month ago (2013-02-22 08:38:38 UTC) #8
minux1
I wonder if we could define DialOption as an interface? type DialOption interface { // ...
11 years, 1 month ago (2013-02-22 15:50:07 UTC) #9
bradfitz
On Fri, Feb 22, 2013 at 7:49 AM, minux <minux.ma@gmail.com> wrote: > I wonder if ...
11 years, 1 month ago (2013-02-22 18:50:54 UTC) #10
bradfitz
On Fri, Feb 22, 2013 at 12:38 AM, minux <minux.ma@gmail.com> wrote: > > On Fri, ...
11 years, 1 month ago (2013-02-22 18:53:09 UTC) #11
rsc
Can we just make DialOpt a struct? Making it an interface means adding tons more ...
11 years, 1 month ago (2013-02-22 18:57:58 UTC) #12
bradfitz
On Fri, Feb 22, 2013 at 10:57 AM, Russ Cox <rsc@golang.org> wrote: > Can we ...
11 years, 1 month ago (2013-02-22 19:21:20 UTC) #13
rsc
My main objection to this CL is just the proliferation of API in package net. ...
11 years, 1 month ago (2013-02-22 20:23:16 UTC) #14
bradfitz
On Fri, Feb 22, 2013 at 12:23 PM, Russ Cox <rsc@golang.org> wrote: > My main ...
11 years, 1 month ago (2013-02-22 20:28:33 UTC) #15
rsc
LGTM
11 years, 1 month ago (2013-02-27 16:42:45 UTC) #16
bradfitz
On Wed, Feb 27, 2013 at 8:42 AM, Russ Cox <rsc@golang.org> wrote: > LGTM > ...
11 years, 1 month ago (2013-02-27 16:46:33 UTC) #17
rsc
I think the DialOpt is nicer than the struct. As long as the new API ...
11 years, 1 month ago (2013-02-27 18:00:12 UTC) #18
r
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 ...
11 years, 1 month ago (2013-02-27 18:29:53 UTC) #19
bradfitz
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.
11 years, 1 month ago (2013-02-27 19:51:28 UTC) #20
bradfitz
This is ready for review + submit now. Updated CL description, added LocalAddr, and hid ...
11 years, 1 month ago (2013-02-27 19:52:22 UTC) #21
r
LGTM
11 years, 1 month ago (2013-02-27 19:53:31 UTC) #22
bradfitz
*** Submitted as https://code.google.com/p/go/source/detail?r=54731addb542 *** net: add DialOpt, the extensible Dial w/ options dialer Add ...
11 years, 1 month ago (2013-02-27 19:59:41 UTC) #23
julienschmidt
11 years, 1 month ago (2013-02-28 02:36:44 UTC) #24
Message was sent while issue was closed.
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b