net: Rebuild TCP connection from file descriptor. (issue4306042)

[ful...@gmail.com]

Reviewers: iant, rsc,

Message:
Hello all

Please review the following change.

We've been working on this code to "rebuild" connections from a file
descriptor.

This is useful when passing an open connection between a server process
and a client handler process, or when using systemd's socket activation
on Linux.

An expert should probably look over the changes we've made to the poll
server to see if we've done the right thing. The main problem we needed
to fix there was that Close was calling Shutdown.

Also, the name RebuildTCP probably needs some work. Suggestions
appreciated.

Once we get this change sorted out, we probably need to move on to Unix
sockets too.

Regards

Albert

Description:
net: Rebuild TCP connection from file descriptor.

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

Affected files:
M src/pkg/net/fd.go
M src/pkg/net/ipsock.go
M src/pkg/net/sock.go
M src/pkg/net/tcpsock.go

[ia...@golang.org]

FYI

http://codereview.appspot.com/4306042/diff/2001/src/pkg/net/fd.go
File src/pkg/net/fd.go (right):

http://codereview.appspot.com/4306042/diff/2001/src/pkg/net/fd.go#newcode136
src/pkg/net/fd.go:136: func (s *pollServer) RemoveFD(fd *netFD, key int,
mode int) {
Should arrange for CheckDeadlines to call this new function instead of
running the same code.

http://codereview.appspot.com/4306042/diff/2001/src/pkg/net/fd.go#newcode221
src/pkg/net/fd.go:221: func (s *pollServer) Cancel(nfd *netFD) {
In the current code, you need to call s.Lock before you look at
s.pending.

http://codereview.appspot.com/4306042/diff/2001/src/pkg/net/fd.go#newcode355
src/pkg/net/fd.go:355: fd.closing = true
This seems like a significant change. Right now if one side of a
connection calls Close the other side will see EOF when reading. With
this change it seems that that won't happen.

http://codereview.appspot.com/4306042/

[ful...@gmail.com]

On 2011/03/22 14:30:20, iant wrote:
> http://codereview.appspot.com/4306042/diff/2001/src/pkg/net/fd.go
> File src/pkg/net/fd.go (right):

http://codereview.appspot.com/4306042/diff/2001/src/pkg/net/fd.go#newcode136
> src/pkg/net/fd.go:136: func (s *pollServer) RemoveFD(fd *netFD, key
int, mode
> int) {
> Should arrange for CheckDeadlines to call this new function instead of
running
> the same code.

Fixed.

http://codereview.appspot.com/4306042/diff/2001/src/pkg/net/fd.go#newcode221
> src/pkg/net/fd.go:221: func (s *pollServer) Cancel(nfd *netFD) {
> In the current code, you need to call s.Lock before you look at
s.pending.

Fixed.

http://codereview.appspot.com/4306042/diff/2001/src/pkg/net/fd.go#newcode355
> src/pkg/net/fd.go:355: fd.closing = true
> This seems like a significant change. Right now if one side of a
connection
> calls Close the other side will see EOF when reading. With this
change it seems
> that that won't happen.

Right. We didn't realise that shutdown had that useful feature.

I've been playing with socat and it seems like read on the client still
returns 0 even if the remote end is killed abruptly. Writing to the
broken socket causes a SIGPIPE to be sent to the process and write
returns EPIPE. I assume Go does something with this signal?

Any ideas on the right way to achieve this? A function
PrettyPleaseDontCallShutdown for the net.Conn API? :-)

Regards

Albert

http://codereview.appspot.com/4306042/

[Russ Cox]

Please revert the shutdown changes. Let's focus
this CL on being able to create a net.Conn for a
specific file descriptor.

Instead of RebuildTCP, I would like to see

// FileConn returns a copy of the network
// connection corresponding to the open file f.
// It is the caller's responsibility to close f when finished.
// Closing c does not affect f, and closing f does not affect c.
func FileConn(f *os.File) (c net.Conn, err os.Error)

// FilePacketConn returns a copy of the packet network
// connection corresponding to the open file f.
// It is the caller's responsibility to close f when finished.
// Closing c does not affect f, and closing f does not affect c.
func FilePacketConn(*os.File) (c net.PacketConn, err os.Error)

The implementations should use dup to get a new file
descriptor for the network connection, just as the File
methods on the connection implementations do.

If the caller really needs a *TCPConn, then it can use
a type assertion after the call to FileConn.

Russ

[ful...@gmail.com]

Hello

On 2011/03/22 16:21:38, rsc wrote:
> Please revert the shutdown changes. Let's focus
> this CL on being able to create a net.Conn for a
> specific file descriptor.

> Instead of RebuildTCP, I would like to see

> func FileConn(f *os.File) (c net.Conn, err os.Error)

> func FilePacketConn(*os.File) (c net.PacketConn, err os.Error)

Do you want these functions in the net package or the os package? I
assume the net package.

> The implementations should use dup to get a new file
> descriptor for the network connection, just as the File
> methods on the connection implementations do.
> If the caller really needs a *TCPConn, then it can use
> a type assertion after the call to FileConn.

I like the API, but I don't quite understand how this is going to avoid
the problem with shutdown.

A server accepts a connection. It has an open file descriptor for the
socket. It calls File() on the socket and passes it to a client handler
process using os.StartProcess with the relevant ProcessArgs. The client
calls FilePacketConn and goes on its merry way.

But what does the server do with the accepted socket? If it drops it on
the ground or calls Close, it leads to Shutdown being called, which
breaks the socket that the client handler process has. Unless I'm
mistaken?

Also, is this API going to allow us to reconstitute a UNIX socket? I
think I'm going to need that to fix the problems I ran into a few days
ago when gob became buffered.

Thanks for the help. Really appreciated.

Regards

Albert

http://codereview.appspot.com/4306042/

[Russ Cox]

> I like the API, but I don't quite understand how this is going to avoid
> the problem with shutdown.

It doesn't. Let's leave shutdown for a different CL.

> Also, is this API going to allow us to reconstitute a UNIX socket?

It should support any net.Conn implementation that
package net knows about.

Russ

[ful...@gmail.com]

Hello

I'm working on this today.

Windows folks: does it make any sense to support this API on Windows? As
far as I can see from the StartProcess code, you can't pass more than
std{in,out,err} to a Windows process anyway, so I can't think of where
these functions would be useful.

Regards

Albert

http://codereview.appspot.com/4306042/

[ful...@gmail.com]

Hello

On 2011/03/23 06:02:22, albert.strasheim wrote:
> On 2011/03/22 16:42:35, rsc wrote:
> > > I like the API, but I don't quite understand how this is going to
avoid
> > > the problem with shutdown.
> > It doesn't. Let's leave shutdown for a different CL.
> > > Also, is this API going to allow us to reconstitute a UNIX socket?
> > It should support any net.Conn implementation that
> > package net knows about.

I've run into an issue quite early on.

getsockname doesn't return the information we need to reconstitute all
the socket parameters. For a TCP listen socket it returns:

&{Port:45493 Addr:[127 0 0 1] raw:{Family:0 Port:0 Addr:[0 0 0 0]
Zero:[0 0 0 0 0 0 0 0]}}

The family value would have been useful.

According to man 7 socket on Linux, the way to get the information is to
use getsockopt with the SO_ACCEPTCONN, SO_DOMAIN, SO_PROTOCOL and
SO_TYPE options.

However, SO_DOMAIN and SO_PROTOCOL are only supported since Linux
2.6.32.

Darwin has SO_TYPE and maybe SO_ACCEPTCONN. Not sure about FreeBSD.

It seems we might have to burden the user with providing this
information.

Thoughts appreciated.

Regards

Albert

http://codereview.appspot.com/4306042/

[alex.b...@gmail.com]

On 2011/03/23 06:02:22, albert.strasheim wrote:

> I can see from the StartProcess code, you can't pass more than
std{in,out,err}
> to a Windows process anyway, so I can't think of where these functions
would be
> useful.

I've never done it myself, but you could certainly use DuplicateHandle
Windows api to transfer any file handle from one process into another.

Alex

http://codereview.appspot.com/4306042/

[Russ Cox]

All you need is SO_TYPE to get the type (SOCK_STREAM, SOCK_DGRAM).
The domain can be inferred from the sockaddr
type and the protocol is unused.

Russ

[ful...@gmail.com]

On 2011/03/23 06:44:41, albert.strasheim wrote:
> According to man 7 socket on Linux, the way to get the information is
to use
> getsockopt with the SO_ACCEPTCONN, SO_DOMAIN, SO_PROTOCOL and SO_TYPE
options.

For future reference, SO_ACCEPTCONN seems to do weird stuff, so I'm not
going to use it for now.

Specifically, checking SO_ACCEPTCONN on the two dups with a close in
between returns different results. strace attached.

Also, the comment in Linux's net/core/sock.c is quite ominous:

"Dubious BSD thing... Probably nobody even uses it, but the UNIX
standard wants it for whatever reason... -DaveM"

[pid 2972] listen(3, 128) = 0
[pid 2972] dup(3) = 7
[pid 2972] fcntl(7, F_GETFL) = 0x802 (flags
O_RDWR|O_NONBLOCK)
[pid 2972] fcntl(7, F_SETFL, O_RDWR) = 0
[pid 2972] dup(7) = 8
[pid 2972] getsockopt(8, SOL_SOCKET, SO_TYPE, [1], [4]) = 0
[pid 2972] getsockname(8, {...}, [28]) = 0
[pid 2972] getpeername(8, 0xf84001fe00, [112]) = -1 ENOTCONN (Transport
endpoint is not connected)
[pid 2972] fcntl(8, F_GETFL) = 0x2 (flags O_RDWR)
[pid 2972] fcntl(8, F_SETFL, O_RDWR|O_NONBLOCK) = 0

Returning 1 as expected:

[pid 2972] getsockopt(8, SOL_SOCKET, SO_ACCEPTCONN, [1], [4]) = 0
[pid 2972] shutdown(8, 2 /* send and receive */) = 0
[pid 2972] fcntl(8, F_GETFL) = 0x802 (flags
O_RDWR|O_NONBLOCK)
[pid 2972] fcntl(8, F_SETFL, O_RDWR) = 0
[pid 2972] close(8) = 0
[pid 2972] dup(7) = 8
[pid 2972] getsockopt(8, SOL_SOCKET, SO_TYPE, [1], [4]) = 0
[pid 2972] getsockname(8, {...}, [28]) = 0
[pid 2972] getpeername(8, 0xf84001f7e0, [112]) = -1 ENOTCONN (Transport
endpoint is not connected)
[pid 2972] fcntl(8, F_GETFL) = 0x2 (flags O_RDWR)
[pid 2972] fcntl(8, F_SETFL, O_RDWR|O_NONBLOCK) = 0

Weirdness:

[pid 2972] getsockopt(8, SOL_SOCKET, SO_ACCEPTCONN, [0], [4]) = 0

http://codereview.appspot.com/4306042/

[Russ Cox]

> For future reference, SO_ACCEPTCONN seems to do weird stuff, so I'm not
> going to use it for now.

It's also Linux-specific. If you use SO_TYPE it will work everywhere.

Russ

[Albert Strasheim]

Hello

SO_TYPE and SO_ACCEPTCONN do slightly different things.

SO_TYPE gives SOCK_STREAM, SOCK_DGRAM, etc.

SO_ACCEPTCONN tells you whether you are dealing with a listen socket.
As far as I can tell from grepping through syscall, SO_ACCEPTCONN
exists on linux, darwin and freebsd, but it doesn't seem to work
properly.

Anyway, I'll have an updated patch ready within the next few minutes.

Regards

Albert

[Russ Cox]

> SO_ACCEPTCONN tells you whether you are dealing with a listen socket.

Ah! I'd forgotten about needing to figure that out.

> As far as I can tell from grepping through syscall, SO_ACCEPTCONN
> exists on linux, darwin and freebsd, but it doesn't seem to work
> properly.

It's undocumented on OS X but does appear in the headers.
I don't know what that means.

Can you use the error return from getpeername
to distinguish the two cases?

Russ

[ful...@gmail.com]

PTAL.

I've written some tests which pass, and I also modified server_test.go
locally to call FileConn, FilePacketConn and FileListener on the sockets
it creates. Everything seems to work.

The only problem I foresee for the future is that one will probably have
to use getsockopt with SO_PROTOCOL to distinguish between SCTP and
TCP/UDP, if SCTP eventually gets added to Go.

The getpeername plan is a good one, but I'm worried that it might return
ENOTCONN for a dialed socket that gets disconnected before FileConn is
called, yielding a false "FileConn called on a listen socket" error. But
maybe that's fine?

Also, I could probably fill in a more useful value for newFD's net
parameter if you think that's necessary.

http://codereview.appspot.com/4306042/

[r...@golang.org]

LGTM

I like how clean this is.
And you don't need SO_ACCEPTCONN after all really... :-)

http://codereview.appspot.com/4306042/diff/15001/src/pkg/net/file.go
File src/pkg/net/file.go (right):

http://codereview.appspot.com/4306042/diff/15001/src/pkg/net/file.go#newcode13
src/pkg/net/file.go:13: s, errno := syscall.Dup(f.Fd())
s/s/fd/

http://codereview.appspot.com/4306042/diff/15001/src/pkg/net/file.go#newcode53
src/pkg/net/file.go:53: return newFD(s, 0, proto, "", laddr, raddr)
s/""/laddr.Network()/

http://codereview.appspot.com/4306042/diff/15001/src/pkg/net/file.go#newcode81
src/pkg/net/file.go:81: // when finished. Closing c does not affect f,

and closing f does not

s/c/l/ next line too

http://codereview.appspot.com/4306042/diff/15001/src/pkg/net/file_test.go
File src/pkg/net/file_test.go (right):

http://codereview.appspot.com/4306042/diff/15001/src/pkg/net/file_test.go#newcode122
src/pkg/net/file_test.go:122: testFilePacketConnListen(t, "udp",
"0.0.0.0:0")
please don't listen on anything other than loopback.
it will trigger the OS X firewall.
127.0.0.1 and [::1] only please

http://codereview.appspot.com/4306042/

[ful...@gmail.com]

PTAL

On 2011/03/23 15:15:11, rsc wrote:
> LGTM

> I like how clean this is.
> And you don't need SO_ACCEPTCONN after all really... :-)

> http://codereview.appspot.com/4306042/diff/15001/src/pkg/net/file.go
> File src/pkg/net/file.go (right):

http://codereview.appspot.com/4306042/diff/15001/src/pkg/net/file.go#newcode13
> src/pkg/net/file.go:13: s, errno := syscall.Dup(f.Fd())
> s/s/fd/

Fixed.

http://codereview.appspot.com/4306042/diff/15001/src/pkg/net/file.go#newcode53
> src/pkg/net/file.go:53: return newFD(s, 0, proto, "", laddr, raddr)
> s/""/laddr.Network()/

Fixed.

http://codereview.appspot.com/4306042/diff/15001/src/pkg/net/file.go#newcode81
> src/pkg/net/file.go:81: // when finished. Closing c does not affect
f, and
> closing f does not
> s/c/l/ next line too

Fixed.

http://codereview.appspot.com/4306042/diff/15001/src/pkg/net/file_test.go
> File src/pkg/net/file_test.go (right):

http://codereview.appspot.com/4306042/diff/15001/src/pkg/net/file_test.go#newcode122
> src/pkg/net/file_test.go:122: testFilePacketConnListen(t, "udp",
"0.0.0.0:0")
> please don't listen on anything other than loopback.
> it will trigger the OS X firewall.
> 127.0.0.1 and [::1] only please

Fixed I think.

http://codereview.appspot.com/4306042/