Code review - Issue 6575050: code review 6575050: net: close dialing fds in DialTimeouthttps://codereview.appspot.com/2012-10-04T15:09:44+00:00rietveld
Message from unknown
2012-09-26T17:34:27+00:00bradfitzurn:md5:1e2b5d46ef8171e0ada178f1eb425ea3
Message from unknown
2012-09-26T17:34:30+00:00bradfitzurn:md5:3dc45596861c3737708bdd350091a81f
Message from unknown
2012-09-26T17:35:19+00:00bradfitzurn:md5:582e8356f394907cc21bd42cabea68d7
Message from bradfitz@golang.org
2012-09-26T17:35:28+00:00bradfitzurn:md5:5ff9077b0e980648f8cceb706f127db3
Hello golang-dev@googlegroups.com,
I'd like you to review this change to
https://go.googlecode.com/hg/
Message from bradfitz@golang.org
2012-09-26T17:59:21+00:00bradfitzurn:md5:84031c099c6854d3d6e686cb83f8bc20
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
> }
>
>
>
Message from rsc@golang.org
2012-09-26T18:52:27+00:00rscurn:md5:4a346ef007b33d9188ac8990bc389b05
Integrating with the poll server should be straightforward. In fact it is probably simpler than using a goroutine.
Message from alex.brainman@gmail.com
2012-09-27T02:38:37+00:00brainmanurn:md5:07cbb3590a7204225ee1684f785fcf49
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
Message from paul@vanbrouwershaven.com
2012-10-02T17:16:05+00:00Paul van Brouwershavenurn:md5:692616cbdce86fd59bca29136248b69b
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
Message from bradfitz@golang.org
2012-10-03T15:04:53+00:00bradfitzurn:md5:ac7475fb03ef7ca69b3ec76d5aca027d
*** Abandoned ***
Message from paul@vanbrouwershaven.com
2012-10-04T15:09:44+00:00Paul van Brouwershavenurn:md5:26b67f90ebf0626b573fa688aa01bda0
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/
>