code review 6815049: net: close fds eagerly in DialTimeout

net: close fds eagerly in DialTimeout

Integrates with the pollserver now.

Uses the old implementation on windows and plan9.

Fixes issue 2631

[bradfitz, 2012-10-29 12:50:09]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2012-10-29 12:54:17]

Thoughts?

I don't entirely like how many parameters the internal functions socket
and internetSocket take, but they were already gross, and it's private, so
I don't mind too much.

I toyed with keeping the parameter counts the same but taking advantage of
raddr being an inteface type (syscall.Sockaddr) and instead just shoving
through the deadline in an wider runtime-asserted interface type (e.g.
deadlineSockaddr), but that was too clever and hard to follow.

This way at least is explicit.

If this looks okay, I'll finish it (not operating systems and socket types).


On Mon, Oct 29, 2012 at 1:50 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: close fds eagerly in DialTimeout
>
> WIP FOR DISCUSSION; THIS VERSION NOT FOR SUBMISSION
>
> Integrates with the pollserver now.
>
> Only currently tested on Linux and I've only plumbed through
> TCP for now.  The rest will be done before submission, as well
> as using the deadline for DNS resolution in DialTimeout.
>
> Fixes issue 2631
>
> Please review this at
http://codereview.appspot.com/**6815049/<http://codereview.appspot.com/6815049/>
>
> Affected files:
>   M src/pkg/net/dial.go
>   M src/pkg/net/dial_test.go
>   M src/pkg/net/iprawsock_posix.go
>   M src/pkg/net/ipsock_posix.go
>   M src/pkg/net/sock_posix.go
>   M src/pkg/net/tcpsock_posix.go
>   M src/pkg/net/udpsock_posix.go
>   M src/pkg/net/unixsock_posix.go
>
>
>

[Paul van Brouwershaven, 2012-10-29 14:32:39]

I tested DialTimeout with this patch applied but it does not release the file
descriptor nor the tcp session when connecting to a non existing IP address.

Console 1: (_, err := net.DialTimeout("tcp", "192.168.178.33:443",
4*time.Second))

2012/10/29 07:24:49 Start
2012/10/29 07:24:53 dial tcp 192.168.178.33:443: i/o timeout
2012/10/29 07:24:53 Sleep: 2500

Console 2: (loop of: "netstat -tn | grep SYN_SENT | wc -l;" / "/usr/bin/lsof -n
| grep a.out | grep IPv4 | wc -l;")

Mon Oct 29 07:24:49 PDT 2012
0
Mon Oct 29 07:24:50 PDT 2012
1
......
Mon Oct 29 07:25:55 PDT 2012
1
Mon Oct 29 07:25:56 PDT 2012
0
Mon Oct 29 07:25:58 PDT 2012

[iant2, 2012-10-29 16:36:07]

On Mon, Oct 29, 2012 at 5:54 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Thoughts?
>
> I don't entirely like how many parameters the internal functions socket and
> internetSocket take, but they were already gross, and it's private, so I
> don't mind too much.
>
> I toyed with keeping the parameter counts the same but taking advantage of
> raddr being an inteface type (syscall.Sockaddr) and instead just shoving
> through the deadline in an wider runtime-asserted interface type (e.g.
> deadlineSockaddr), but that was too clever and hard to follow.
>
> This way at least is explicit.
>
> If this looks okay, I'll finish it (not operating systems and socket types).


Looks reasonable to me.

Ian

[bradfitz, 2012-10-29 16:39:03]

On Mon, Oct 29, 2012 at 3:32 PM, <paul@vanbrouwershaven.com> wrote:

> I tested DialTimeout with this patch applied but it does not release the
> file descriptor nor the tcp session when connecting to a non existing IP
> address.
>

It shouldn't matter whether it exists or not.

My tests pass with the fix and fail without the fix, so I fixed something.

But adding some debug prints, I'm not seeing the explicit socket close I
expected, so something's still missing.  I bet the finalizers are closing
my sockets, which might explain why my tests pass but your single fd test
fails.

I'll look into it more and expand the tests.

[bradfitz, 2012-10-30 17:16:24]

On Mon, Oct 29, 2012 at 3:32 PM, <paul@vanbrouwershaven.com> wrote:

> I tested DialTimeout with this patch applied but it does not release the
> file descriptor nor the tcp session when connecting to a non existing IP
> address.
>

I found the problem.  Thanks for pointing it out.

[adg, 2012-10-31 07:36:23]

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

https://codereview.appspot.com/6815049/diff/3009/src/pkg/net/dial.go#newcode96
src/pkg/net/dial.go:96: return dialAddr(net, addr, addri, time.Time{})
var deadline time.Time
return dialAddr(net, addr, addri, deadline)

[bradfitz, 2012-10-31 08:22:20]

I have a noDeadline variable in the latest version (not yet mailed), which
is more clear.

On Wed, Oct 31, 2012 at 8:36 AM, <adg@golang.org> wrote:

>
>
https://codereview.appspot.**com/6815049/diff/3009/src/pkg/**net/dial.go<http...
> File src/pkg/net/dial.go (right):
>
> https://codereview.appspot.**com/6815049/diff/3009/src/pkg/**
>
net/dial.go#newcode96<https://codereview.appspot.com/6815049/diff/3009/src/pkg/net/dial.go#newcode96>
> src/pkg/net/dial.go:96: return dialAddr(net, addr, addri, time.Time{})
> var deadline time.Time
> return dialAddr(net, addr, addri, deadline)
>
>
https://codereview.appspot.**com/6815049/<https://codereview.appspot.com/6815...
>

[bradfitz, 2012-10-31 11:01:07]

Hello golang-dev@googlegroups.com, paul@vanbrouwershaven.com, iant@google.com,
adg@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-10-31 11:01:31]

This is ready for review now.

On Wed, Oct 31, 2012 at 12:01 PM, <bradfitz@golang.org> wrote:

> Hello golang-dev@googlegroups.com, paul@vanbrouwershaven.com,
> iant@google.com, adg@golang.org (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**6815049/<http://codereview.appspot.com/6815049/>
>

[Paul van Brouwershaven, 2012-10-31 13:46:50]

I tested this code and it has passed all my initial tests! Thanks for all the
hard work I'm really happy that this one is fixed!

Now it becomes the challenge to apply this same logic to DialTCP. Are there any
plans to create a DialTCPTimeout?

On 2012/10/31 11:01:31, bradfitz wrote:
> This is ready for review now.
> 
> On Wed, Oct 31, 2012 at 12:01 PM, <mailto:bradfitz@golang.org> wrote:
> 
> > Hello mailto:golang-dev@googlegroups.com, mailto:paul@vanbrouwershaven.com,
> > mailto:iant@google.com, mailto:adg@golang.org (cc:
mailto:golang-dev@googlegroups.com),
> >
> > Please take another look.
> >
> >
> >
>
http://codereview.appspot.com/**6815049/%3Chttp://codereview.appspot.com/6815...>
> >

[bradfitz, 2012-10-31 13:51:51]

That would be one line of code but continues an API proliferation
precedent. Can't you just use DialTimeout or do you need to set the local
address?
On Oct 31, 2012 2:46 PM, <paul@vanbrouwershaven.com> wrote:

> I tested this code and it has passed all my initial tests! Thanks for
> all the hard work I'm really happy that this one is fixed!
>
> Now it becomes the challenge to apply this same logic to DialTCP. Are
> there any plans to create a DialTCPTimeout?
>
> On 2012/10/31 11:01:31, bradfitz wrote:
>
>> This is ready for review now.
>>
>
>  On Wed, Oct 31, 2012 at 12:01 PM, <mailto:bradfitz@golang.org> wrote:
>>
>
>  > Hello mailto:golang-dev@**googlegroups.com<golang-dev@googlegroups.com>
>> ,
>>
> mailto:paul@vanbrouwershaven.**com <paul@vanbrouwershaven.com>,
>
>> > mailto:iant@google.com, mailto:adg@golang.org (cc:
>>
> mailto:golang-dev@**googlegroups.com <golang-dev@googlegroups.com>),
>
>> >
>> > Please take another look.
>> >
>> >
>> >
>>
>
> http://codereview.appspot.com/****6815049/%3Chttp://**
>
codereview.appspot.com/**6815049/<http://codereview.appspot.com/**6815049/%3Chttp://codereview.appspot.com/6815049/>
> >
>
>> >
>>
>
>
>
https://codereview.appspot.**com/6815049/<https://codereview.appspot.com/6815...
>

[Paul van Brouwershaven, 2012-10-31 13:59:45]

Unfortunately, I need to set the local address which can only be done with
DialTCP for now.

On 2012/10/31 13:51:51, bradfitz wrote:
> That would be one line of code but continues an API proliferation
> precedent. Can't you just use DialTimeout or do you need to set the local
> address?

[Paul van Brouwershaven, 2012-10-31 15:12:37]

The best way to implement this would be by using a setter as it's an infrequent
used parameter. I haven't seen setters on GO, so this may not be an option? 

To continue testing on a broader scale I implemented a temporary DialTCPTimeout
function in src/pkg/net/dial.go (see below) and till now the patch it's exactly
doing what it should do!

The number of fd / SYN_SENT connections is dropped exactly to the number of
concurrent sessions.

--------------------
// DialTCPTimeout acts like DialTCP but takes a timeout.
// The timeout includes name resolution, if required.
func DialTCPTimeout(net, laddr, raddr string, timeout time.Duration) (Conn,
error) {
    deadline := time.Now().Add(timeout)
	laddri, err := ResolveTCPAddr("tcp", laddr)
	if err != nil {
		return nil, err
	}
    raddri, err := ResolveTCPAddr("tcp", raddr)
	if err != nil {
		return nil, err
	}
    return dialTCP(net, laddri, raddri, deadline)
}

[bradfitz, 2012-10-31 15:14:39]

On Wed, Oct 31, 2012 at 4:12 PM, <paul@vanbrouwershaven.com> wrote:

> The best way to implement this would be by using a setter as it's an
> infrequent used parameter. I haven't seen setters on GO, so this may not
> be an option?
>

See API discussion at the bottom of semi-related
http://code.google.com/p/go/issues/detail?id=3610

[BenDaglish, 2012-10-31 22:32:58]

Just to confirm, I had the same problem, and this patch works fine on Centos5
x86_64

[rsc, 2012-11-01 18:39:44]

Seems fine so far.

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

https://codereview.appspot.com/6815049/diff/5028/src/pkg/net/dial.go#newcode53
src/pkg/net/dial.go:53: // TODO(bradfitz): consider pushing the deadline down
into the
It doesn't need to be pushed into the actual DNS lookup, but please do push it
down into unexported resolveTCPAddr, resolveUDPAddr, etc. If the address is
"1.2.3.4:80" it would be nice not to race a timing goroutine against the parsing
of the IP address.

https://codereview.appspot.com/6815049/diff/5028/src/pkg/net/dial.go#newcode152
src/pkg/net/dial.go:152: var useDialTimeoutRace = runtime.GOOS == "windows" ||
runtime.GOOS == "plan9"
Might as well make this const

https://codereview.appspot.com/6815049/diff/5028/src/pkg/net/tcpsock_plan9.go
File src/pkg/net/tcpsock_plan9.go (right):

https://codereview.appspot.com/6815049/diff/5028/src/pkg/net/tcpsock_plan9.go...
src/pkg/net/tcpsock_plan9.go:48: panic("Not implemented, but shouldn't get here.
See dial.go")
panic("net.dialTCP: deadline not implemented on Plan 9")

https://codereview.appspot.com/6815049/diff/5028/src/pkg/net/udpsock_plan9.go
File src/pkg/net/udpsock_plan9.go (right):

https://codereview.appspot.com/6815049/diff/5028/src/pkg/net/udpsock_plan9.go...
src/pkg/net/udpsock_plan9.go:132: panic("Not implemented, but shouldn't get
here. See dial.go")
panic("net.dialUDP: deadline not implemented on Plan 9")

[bradfitz, 2012-11-02 09:33:49]

Hello paul@vanbrouwershaven.com, iant@google.com, adg@golang.org,
bendaglish@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-11-02 09:35:02]

PTAL

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

https://codereview.appspot.com/6815049/diff/5028/src/pkg/net/dial.go#newcode53
src/pkg/net/dial.go:53: // TODO(bradfitz): consider pushing the deadline down
into the
On 2012/11/01 18:39:44, rsc wrote:
> It doesn't need to be pushed into the actual DNS lookup, but please do push it
> down into unexported resolveTCPAddr, resolveUDPAddr, etc. If the address is
> "1.2.3.4:80" it would be nice not to race a timing goroutine against the
parsing
> of the IP address.

Done.

I'd actually started on this from the other direction and had a bunch of
deadlines propagated down below but not hooked up yet.  I connected the
remaining dots.

https://codereview.appspot.com/6815049/diff/5028/src/pkg/net/dial.go#newcode152
src/pkg/net/dial.go:152: var useDialTimeoutRace = runtime.GOOS == "windows" ||
runtime.GOOS == "plan9"
On 2012/11/01 18:39:44, rsc wrote:
> Might as well make this const

Done.

https://codereview.appspot.com/6815049/diff/5028/src/pkg/net/tcpsock_plan9.go
File src/pkg/net/tcpsock_plan9.go (right):

https://codereview.appspot.com/6815049/diff/5028/src/pkg/net/tcpsock_plan9.go...
src/pkg/net/tcpsock_plan9.go:48: panic("Not implemented, but shouldn't get here.
See dial.go")
On 2012/11/01 18:39:44, rsc wrote:
> panic("net.dialTCP: deadline not implemented on Plan 9")

Done.

https://codereview.appspot.com/6815049/diff/5028/src/pkg/net/udpsock_plan9.go
File src/pkg/net/udpsock_plan9.go (right):

https://codereview.appspot.com/6815049/diff/5028/src/pkg/net/udpsock_plan9.go...
src/pkg/net/udpsock_plan9.go:132: panic("Not implemented, but shouldn't get
here. See dial.go")
On 2012/11/01 18:39:44, rsc wrote:
> panic("net.dialUDP: deadline not implemented on Plan 9")

Done.

[Paul van Brouwershaven, 2012-11-08 07:48:29]

I get and error when applying patch set 7, applying the failed patch by hand
does not solve all problems.

Hunk #1 succeeded at 162 (offset -1 lines).
Hunk #2 succeeded at 187 (offset -1 lines).
Hunk #3 succeeded at 332 (offset 4 lines).
patching file src/pkg/net/iprawsock.go
patching file src/pkg/net/iprawsock_plan9.go
patching file src/pkg/net/iprawsock_posix.go
patching file src/pkg/net/ipsock.go
Hunk #1 FAILED at 6.
Hunk #2 succeeded at 103 (offset 5 lines).
Hunk #3 succeeded at 122 (offset 5 lines).
1 out of 3 hunks FAILED -- saving rejects to file src/pkg/net/ipsock.go.rej

------------------------------------------------------------------------------
--- src/pkg/net/ipsock.go
+++ src/pkg/net/ipsock.go
@@ -6,6 +6,8 @@

 package net

+import "time"
+
 var supportsIPv6, supportsIPv4map = probeIPv6Stack()

 func firstFavoriteAddr(filter func(IP) IP, addrs []string) (addr IP) {

------------------------------------------------------------------------------

Adding 'import "time"' to 'src/pkg/net/ipsock.go' will no solve the whole
problem. Running all.bash causes the following error:

# Checking API compatibility.
2012/11/07 23:42:09 unknown kind in const "useDialTimeoutRace"
(*ast.BinaryExpr): in BinaryExpr, unhandled type mismatch; left="string",
right="ideal-string"
go tool api: exit status 1

------------------------------------------------------------------------------

Patch set 6 now causes the same error for adding 'import "time"' to
'src/pkg/net/ipsock.go'. Patching this file by hand makes the compiler runs fine
for patch set 6.

[bradfitz, 2012-11-08 16:35:19]

*** Submitted as http://code.google.com/p/go/source/detail?r=20319e86e3c9 ***

net: close fds eagerly in DialTimeout

Integrates with the pollserver now.

Uses the old implementation on windows and plan9.

Fixes issue 2631

R=paul, iant, adg, bendaglish, rsc
CC=golang-dev
http://codereview.appspot.com/6815049