FAQ
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 dialing fds in DialTimeout

WIP. (Kinda gross) For discussion.

Fixes issue 2631

Please review this at http://codereview.appspot.com/6575050/

Affected files:
M src/pkg/net/dial.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


Index: src/pkg/net/dial.go
===================================================================
--- a/src/pkg/net/dial.go
+++ b/src/pkg/net/dial.go
@@ -93,13 +93,13 @@
if err != nil {
return nil, err
}
- return dialAddr(net, addr, addri)
+ return dialAddr(net, addr, addri, nil)
}

-func dialAddr(net, addr string, addri Addr) (c Conn, err error) {
+func dialAddr(net, addr string, addri Addr, beforeConnect chan<- *netFD)
(c Conn, err error) {
switch ra := addri.(type) {
case *TCPAddr:
- c, err = DialTCP(net, nil, ra)
+ c, err = dialTCP(net, nil, ra, beforeConnect)
case *UDPAddr:
c, err = DialUDP(net, nil, ra)
case *IPAddr:
@@ -130,6 +130,7 @@
}
ch := make(chan pair, 1)
resolvedAddr := make(chan Addr, 1)
+ fdc := make(chan *netFD, 1)
go func() {
_, addri, err := resolveNetAddr("dial", net, addr)
if err != nil {
@@ -137,7 +138,7 @@
return
}
resolvedAddr <- addri // in case we need it for OpError
- c, err := dialAddr(net, addr, addri)
+ c, err := dialAddr(net, addr, addri, fdc)
ch <- pair{c, err}
}()
select {
@@ -157,6 +158,14 @@
Addr: addri,
Err: &timeoutError{},
}
+
+ // Try to close the *netFD, if it was sent.
+ select {
+ case fd := <-fdc:
+ fd.Close()
+ default:
+ }
+
return nil, err
case p := <-ch:
return p.Conn, p.error
Index: src/pkg/net/iprawsock_posix.go
===================================================================
--- a/src/pkg/net/iprawsock_posix.go
+++ b/src/pkg/net/iprawsock_posix.go
@@ -175,7 +175,7 @@
if raddr == nil {
return nil, &OpError{"dial", netProto, nil, errMissingAddress}
}
- fd, err := internetSocket(net, laddr.toAddr(), raddr.toAddr(),
syscall.SOCK_RAW, proto, "dial", sockaddrToIP)
+ fd, err := internetSocket(net, laddr.toAddr(), raddr.toAddr(),
syscall.SOCK_RAW, proto, "dial", sockaddrToIP, nil)
if err != nil {
return nil, err
}
@@ -196,7 +196,7 @@
default:
return nil, UnknownNetworkError(net)
}
- fd, err := internetSocket(net, laddr.toAddr(), nil, syscall.SOCK_RAW,
proto, "listen", sockaddrToIP)
+ fd, err := internetSocket(net, laddr.toAddr(), nil, syscall.SOCK_RAW,
proto, "listen", sockaddrToIP, nil)
if err != nil {
return nil, err
}
Index: src/pkg/net/ipsock_posix.go
===================================================================
--- a/src/pkg/net/ipsock_posix.go
+++ b/src/pkg/net/ipsock_posix.go
@@ -125,7 +125,7 @@
sockaddr(family int) (syscall.Sockaddr, error)
}

-func internetSocket(net string, laddr, raddr sockaddr, sotype, proto int,
mode string, toAddr func(syscall.Sockaddr) Addr) (fd *netFD, err error) {
+func internetSocket(net string, laddr, raddr sockaddr, sotype, proto int,
mode string, toAddr func(syscall.Sockaddr) Addr, beforeConnect chan<-
*netFD) (fd *netFD, err error) {
var la, ra syscall.Sockaddr
family, ipv6only := favoriteAddrFamily(net, laddr, raddr, mode)
if laddr != nil {
@@ -138,7 +138,7 @@
goto Error
}
}
- fd, err = socket(net, family, sotype, proto, ipv6only, la, ra, toAddr)
+ fd, err = socket(net, family, sotype, proto, ipv6only, la, ra, toAddr,
beforeConnect)
if err != nil {
goto Error
}
Index: src/pkg/net/sock_posix.go
===================================================================
--- a/src/pkg/net/sock_posix.go
+++ b/src/pkg/net/sock_posix.go
@@ -16,7 +16,7 @@
var listenerBacklog = maxListenerBacklog()

// Generic socket creation.
-func socket(net string, f, t, p int, ipv6only bool, ulsa, ursa
syscall.Sockaddr, toAddr func(syscall.Sockaddr) Addr) (fd *netFD, err
error) {
+func socket(net string, f, t, p int, ipv6only bool, ulsa, ursa
syscall.Sockaddr, toAddr func(syscall.Sockaddr) Addr, beforeConnect chan<-
*netFD) (fd *netFD, err error) {
// See ../syscall/exec_unix.go for description of ForkLock.
syscall.ForkLock.RLock()
s, err := syscall.Socket(f, t, p)
@@ -50,6 +50,9 @@
}

if ursa != nil {
+ if beforeConnect != nil {
+ beforeConnect <- fd
+ }
if err = fd.connect(ursa); err != nil {
closesocket(s)
fd.Close()
Index: src/pkg/net/tcpsock_posix.go
===================================================================
--- a/src/pkg/net/tcpsock_posix.go
+++ b/src/pkg/net/tcpsock_posix.go
@@ -143,11 +143,16 @@
// which must be "tcp", "tcp4", or "tcp6". If laddr is not nil, it is used
// as the local address for the connection.
func DialTCP(net string, laddr, raddr *TCPAddr) (*TCPConn, error) {
+ return dialTCP(net, laddr, raddr, nil)
+}
+
+// If beforeConnect is non-nil, it receives the *netFD before connect is
called.
+func dialTCP(net string, laddr, raddr *TCPAddr, beforeConnect chan<-
*netFD) (*TCPConn, error) {
if raddr == nil {
return nil, &OpError{"dial", net, nil, errMissingAddress}
}

- fd, err := internetSocket(net, laddr.toAddr(), raddr.toAddr(),
syscall.SOCK_STREAM, 0, "dial", sockaddrToTCP)
+ fd, err := internetSocket(net, laddr.toAddr(), raddr.toAddr(),
syscall.SOCK_STREAM, 0, "dial", sockaddrToTCP, beforeConnect)

// TCP has a rarely used mechanism called a 'simultaneous connection' in
// which Dial("tcp", addr1, addr2) run on the machine at addr1 can
@@ -177,7 +182,7 @@
if err == nil {
fd.Close()
}
- fd, err = internetSocket(net, laddr.toAddr(), raddr.toAddr(),
syscall.SOCK_STREAM, 0, "dial", sockaddrToTCP)
+ fd, err = internetSocket(net, laddr.toAddr(), raddr.toAddr(),
syscall.SOCK_STREAM, 0, "dial", sockaddrToTCP, nil)
}

if err != nil {
@@ -225,7 +230,7 @@
// If laddr has a port of 0, it means to listen on some available port.
// The caller can use l.Addr() to retrieve the chosen address.
func ListenTCP(net string, laddr *TCPAddr) (*TCPListener, error) {
- fd, err := internetSocket(net, laddr.toAddr(), nil, syscall.SOCK_STREAM,
0, "listen", sockaddrToTCP)
+ fd, err := internetSocket(net, laddr.toAddr(), nil, syscall.SOCK_STREAM,
0, "listen", sockaddrToTCP, nil)
if err != nil {
return nil, err
}
Index: src/pkg/net/udpsock_posix.go
===================================================================
--- a/src/pkg/net/udpsock_posix.go
+++ b/src/pkg/net/udpsock_posix.go
@@ -168,7 +168,7 @@
if raddr == nil {
return nil, &OpError{"dial", net, nil, errMissingAddress}
}
- fd, err := internetSocket(net, laddr.toAddr(), raddr.toAddr(),
syscall.SOCK_DGRAM, 0, "dial", sockaddrToUDP)
+ fd, err := internetSocket(net, laddr.toAddr(), raddr.toAddr(),
syscall.SOCK_DGRAM, 0, "dial", sockaddrToUDP, nil)
if err != nil {
return nil, err
}
@@ -188,7 +188,7 @@
if laddr == nil {
return nil, &OpError{"listen", net, nil, errMissingAddress}
}
- fd, err := internetSocket(net, laddr.toAddr(), nil, syscall.SOCK_DGRAM,
0, "listen", sockaddrToUDP)
+ fd, err := internetSocket(net, laddr.toAddr(), nil, syscall.SOCK_DGRAM,
0, "listen", sockaddrToUDP, nil)
if err != nil {
return nil, err
}
@@ -208,7 +208,7 @@
if gaddr == nil || gaddr.IP == nil {
return nil, &OpError{"listenmulticast", net, nil, errMissingAddress}
}
- fd, err := internetSocket(net, gaddr.toAddr(), nil, syscall.SOCK_DGRAM,
0, "listen", sockaddrToUDP)
+ fd, err := internetSocket(net, gaddr.toAddr(), nil, syscall.SOCK_DGRAM,
0, "listen", sockaddrToUDP, nil)
if err != nil {
return nil, err
}
Index: src/pkg/net/unixsock_posix.go
===================================================================
--- a/src/pkg/net/unixsock_posix.go
+++ b/src/pkg/net/unixsock_posix.go
@@ -59,7 +59,7 @@
f = sockaddrToUnixpacket
}

- fd, err = socket(net, syscall.AF_UNIX, sotype, 0, false, la, ra, f)
+ fd, err = socket(net, syscall.AF_UNIX, sotype, 0, false, la, ra, f, nil)
if err != nil {
goto Error
}

Search Discussions

  • Brad Fitzpatrick at Sep 26, 2012 at 5:59 pm
    FWIW, I implemented this for somebody who mailed me privately saying this
    was a show-stopped for their application.

    I offered this CL so they can test it out and see if it fixes their
    observed problems.

    I can imagine several different ways to implement this. Rather than
    closing the socket from another goroutine, we could also integrate with the
    pollserver and set timeouts there.

    This also lacks a test to see if the fds go away.

    On Wed, Sep 26, 2012 at 10:35 AM, 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 dialing fds in DialTimeout

    WIP. (Kinda gross) For discussion.

    Fixes issue 2631

    Please review this at http://codereview.appspot.com/**6575050/<http://codereview.appspot.com/6575050/>

    Affected files:
    M src/pkg/net/dial.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


    Index: src/pkg/net/dial.go
    ==============================**==============================**=======
    --- a/src/pkg/net/dial.go
    +++ b/src/pkg/net/dial.go
    @@ -93,13 +93,13 @@
    if err != nil {
    return nil, err
    }
    - return dialAddr(net, addr, addri)
    + return dialAddr(net, addr, addri, nil)
    }

    -func dialAddr(net, addr string, addri Addr) (c Conn, err error) {
    +func dialAddr(net, addr string, addri Addr, beforeConnect chan<- *netFD)
    (c Conn, err error) {
    switch ra := addri.(type) {
    case *TCPAddr:
    - c, err = DialTCP(net, nil, ra)
    + c, err = dialTCP(net, nil, ra, beforeConnect)
    case *UDPAddr:
    c, err = DialUDP(net, nil, ra)
    case *IPAddr:
    @@ -130,6 +130,7 @@
    }
    ch := make(chan pair, 1)
    resolvedAddr := make(chan Addr, 1)
    + fdc := make(chan *netFD, 1)
    go func() {
    _, addri, err := resolveNetAddr("dial", net, addr)
    if err != nil {
    @@ -137,7 +138,7 @@
    return
    }
    resolvedAddr <- addri // in case we need it for OpError
    - c, err := dialAddr(net, addr, addri)
    + c, err := dialAddr(net, addr, addri, fdc)
    ch <- pair{c, err}
    }()
    select {
    @@ -157,6 +158,14 @@
    Addr: addri,
    Err: &timeoutError{},
    }
    +
    + // Try to close the *netFD, if it was sent.
    + select {
    + case fd := <-fdc:
    + fd.Close()
    + default:
    + }
    +
    return nil, err
    case p := <-ch:
    return p.Conn, p.error
    Index: src/pkg/net/iprawsock_posix.go
    ==============================**==============================**=======
    --- a/src/pkg/net/iprawsock_posix.**go
    +++ b/src/pkg/net/iprawsock_posix.**go
    @@ -175,7 +175,7 @@
    if raddr == nil {
    return nil, &OpError{"dial", netProto, nil,
    errMissingAddress}
    }
    - fd, err := internetSocket(net, laddr.toAddr(), raddr.toAddr(),
    syscall.SOCK_RAW, proto, "dial", sockaddrToIP)
    + fd, err := internetSocket(net, laddr.toAddr(), raddr.toAddr(),
    syscall.SOCK_RAW, proto, "dial", sockaddrToIP, nil)
    if err != nil {
    return nil, err
    }
    @@ -196,7 +196,7 @@
    default:
    return nil, UnknownNetworkError(net)
    }
    - fd, err := internetSocket(net, laddr.toAddr(), nil,
    syscall.SOCK_RAW, proto, "listen", sockaddrToIP)
    + fd, err := internetSocket(net, laddr.toAddr(), nil,
    syscall.SOCK_RAW, proto, "listen", sockaddrToIP, nil)
    if err != nil {
    return nil, err
    }
    Index: src/pkg/net/ipsock_posix.go
    ==============================**==============================**=======
    --- a/src/pkg/net/ipsock_posix.go
    +++ b/src/pkg/net/ipsock_posix.go
    @@ -125,7 +125,7 @@
    sockaddr(family int) (syscall.Sockaddr, error)
    }

    -func internetSocket(net string, laddr, raddr sockaddr, sotype, proto int,
    mode string, toAddr func(syscall.Sockaddr) Addr) (fd *netFD, err error) {
    +func internetSocket(net string, laddr, raddr sockaddr, sotype, proto int,
    mode string, toAddr func(syscall.Sockaddr) Addr, beforeConnect chan<-
    *netFD) (fd *netFD, err error) {
    var la, ra syscall.Sockaddr
    family, ipv6only := favoriteAddrFamily(net, laddr, raddr, mode)
    if laddr != nil {
    @@ -138,7 +138,7 @@
    goto Error
    }
    }
    - fd, err = socket(net, family, sotype, proto, ipv6only, la, ra,
    toAddr)
    + fd, err = socket(net, family, sotype, proto, ipv6only, la, ra,
    toAddr, beforeConnect)
    if err != nil {
    goto Error
    }
    Index: src/pkg/net/sock_posix.go
    ==============================**==============================**=======
    --- a/src/pkg/net/sock_posix.go
    +++ b/src/pkg/net/sock_posix.go
    @@ -16,7 +16,7 @@
    var listenerBacklog = maxListenerBacklog()

    // Generic socket creation.
    -func socket(net string, f, t, p int, ipv6only bool, ulsa, ursa
    syscall.Sockaddr, toAddr func(syscall.Sockaddr) Addr) (fd *netFD, err
    error) {
    +func socket(net string, f, t, p int, ipv6only bool, ulsa, ursa
    syscall.Sockaddr, toAddr func(syscall.Sockaddr) Addr, beforeConnect chan<-
    *netFD) (fd *netFD, err error) {
    // See ../syscall/exec_unix.go for description of ForkLock.
    syscall.ForkLock.RLock()
    s, err := syscall.Socket(f, t, p)
    @@ -50,6 +50,9 @@
    }

    if ursa != nil {
    + if beforeConnect != nil {
    + beforeConnect <- fd
    + }
    if err = fd.connect(ursa); err != nil {
    closesocket(s)
    fd.Close()
    Index: src/pkg/net/tcpsock_posix.go
    ==============================**==============================**=======
    --- a/src/pkg/net/tcpsock_posix.go
    +++ b/src/pkg/net/tcpsock_posix.go
    @@ -143,11 +143,16 @@
    // which must be "tcp", "tcp4", or "tcp6". If laddr is not nil, it is
    used
    // as the local address for the connection.
    func DialTCP(net string, laddr, raddr *TCPAddr) (*TCPConn, error) {
    + return dialTCP(net, laddr, raddr, nil)
    +}
    +
    +// If beforeConnect is non-nil, it receives the *netFD before connect is
    called.
    +func dialTCP(net string, laddr, raddr *TCPAddr, beforeConnect chan<-
    *netFD) (*TCPConn, error) {
    if raddr == nil {
    return nil, &OpError{"dial", net, nil, errMissingAddress}
    }

    - fd, err := internetSocket(net, laddr.toAddr(), raddr.toAddr(),
    syscall.SOCK_STREAM, 0, "dial", sockaddrToTCP)
    + fd, err := internetSocket(net, laddr.toAddr(), raddr.toAddr(),
    syscall.SOCK_STREAM, 0, "dial", sockaddrToTCP, beforeConnect)

    // TCP has a rarely used mechanism called a 'simultaneous
    connection' in
    // which Dial("tcp", addr1, addr2) run on the machine at addr1 can
    @@ -177,7 +182,7 @@
    if err == nil {
    fd.Close()
    }
    - fd, err = internetSocket(net, laddr.toAddr(),
    raddr.toAddr(), syscall.SOCK_STREAM, 0, "dial", sockaddrToTCP)
    + fd, err = internetSocket(net, laddr.toAddr(),
    raddr.toAddr(), syscall.SOCK_STREAM, 0, "dial", sockaddrToTCP, nil)
    }

    if err != nil {
    @@ -225,7 +230,7 @@
    // If laddr has a port of 0, it means to listen on some available port.
    // The caller can use l.Addr() to retrieve the chosen address.
    func ListenTCP(net string, laddr *TCPAddr) (*TCPListener, error) {
    - fd, err := internetSocket(net, laddr.toAddr(), nil,
    syscall.SOCK_STREAM, 0, "listen", sockaddrToTCP)
    + fd, err := internetSocket(net, laddr.toAddr(), nil,
    syscall.SOCK_STREAM, 0, "listen", sockaddrToTCP, nil)
    if err != nil {
    return nil, err
    }
    Index: src/pkg/net/udpsock_posix.go
    ==============================**==============================**=======
    --- a/src/pkg/net/udpsock_posix.go
    +++ b/src/pkg/net/udpsock_posix.go
    @@ -168,7 +168,7 @@
    if raddr == nil {
    return nil, &OpError{"dial", net, nil, errMissingAddress}
    }
    - fd, err := internetSocket(net, laddr.toAddr(), raddr.toAddr(),
    syscall.SOCK_DGRAM, 0, "dial", sockaddrToUDP)
    + fd, err := internetSocket(net, laddr.toAddr(), raddr.toAddr(),
    syscall.SOCK_DGRAM, 0, "dial", sockaddrToUDP, nil)
    if err != nil {
    return nil, err
    }
    @@ -188,7 +188,7 @@
    if laddr == nil {
    return nil, &OpError{"listen", net, nil, errMissingAddress}
    }
    - fd, err := internetSocket(net, laddr.toAddr(), nil,
    syscall.SOCK_DGRAM, 0, "listen", sockaddrToUDP)
    + fd, err := internetSocket(net, laddr.toAddr(), nil,
    syscall.SOCK_DGRAM, 0, "listen", sockaddrToUDP, nil)
    if err != nil {
    return nil, err
    }
    @@ -208,7 +208,7 @@
    if gaddr == nil || gaddr.IP == nil {
    return nil, &OpError{"listenmulticast", net, nil,
    errMissingAddress}
    }
    - fd, err := internetSocket(net, gaddr.toAddr(), nil,
    syscall.SOCK_DGRAM, 0, "listen", sockaddrToUDP)
    + fd, err := internetSocket(net, gaddr.toAddr(), nil,
    syscall.SOCK_DGRAM, 0, "listen", sockaddrToUDP, nil)
    if err != nil {
    return nil, err
    }
    Index: src/pkg/net/unixsock_posix.go
    ==============================**==============================**=======
    --- a/src/pkg/net/unixsock_posix.**go
    +++ b/src/pkg/net/unixsock_posix.**go
    @@ -59,7 +59,7 @@
    f = sockaddrToUnixpacket
    }

    - fd, err = socket(net, syscall.AF_UNIX, sotype, 0, false, la, ra, f)
    + fd, err = socket(net, syscall.AF_UNIX, sotype, 0, false, la, ra,
    f, nil)
    if err != nil {
    goto Error
    }

  • Rsc at Sep 26, 2012 at 6:57 pm
    Integrating with the poll server should be straightforward. In fact it
    is probably simpler than using a goroutine.


    http://codereview.appspot.com/6575050/
  • Alex Brainman at Sep 27, 2012 at 2:38 am

    On 2012/09/26 18:52:27, rsc wrote:
    Integrating with the poll server should be straightforward. In fact it is
    probably simpler than using a goroutine.
    I didn't look properly. But, I think, we could use windows ConnectEx api
    to implement similar functionality on windows. It should fit our current
    windows "poll server" method too.

    Alex

    http://codereview.appspot.com/6575050/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedSep 26, '12 at 5:59p
activeSep 27, '12 at 2:38a
posts4
users3
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase