code review 6575050: net: close dialing fds in DialTimeout

net: close dialing fds in DialTimeout

WIP. (Kinda gross) For discussion.

Fixes issue 2631

[bradfitz, 2012-09-26 17:35:28]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2012-09-26 17:59:21]

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, <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 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, 2012-09-26 18:52:27]

Integrating with the poll server should be straightforward. In fact it is
probably simpler than using a goroutine.

[brainman, 2012-09-27 02:38:37]

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

[Paul van Brouwershaven, 2012-10-02 17:16:05]

I did some testing on this code. Initially everything looks fine and the fd is
closed. But as soon I start to make concurrent requests I'm getting the message
"Epoll delete fd=35212: bad file descriptor".

The error does not always occur and I get the feeling that the fd is closed
between the timeout and the close request.

The location of the error message:
http://code.google.com/p/go/source/browse/src/pkg/net/fd_linux.go#111

[bradfitz, 2012-10-03 15:04:53]

*** Abandoned ***

[Paul van Brouwershaven, 2012-10-04 15:09:44]

When a connection fails because a server not listening on that port, 
fd.connect(ursa) does not return an error but sets fd.isConnected to true. 
This causes an open file descriptor till a timeout occurs. I would also 
suggest that this would be the right place to handle all connection 
timeouts.

if err = fd.connect(ursa); err != nil {
closesocket(s)
fd.Close()
return nil, err
}
fd.isConnected = true 
http://code.google.com/p/go/source/browse/src/pkg/net/sock_posix.go#53

Any idea how to detect the connection failure or/and to implement a better 
timeout handler?

On Wednesday, October 3, 2012 5:04:56 PM UTC+2, Brad Fitzpatrick wrote:
>
> *** Abandoned *** 
>
> http://codereview.appspot.com/6575050/ 
>