net: Reconstitute Conn/PacketConn/Listener from os.File.

net: Reconstitute Conn/PacketConn/Listener from os.File.

[albert.strasheim, 2011-03-22 12:57:42]

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

[iant, 2011-03-22 14:30:20]

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.

[albert.strasheim, 2011-03-22 16:11:25]

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

[rsc, 2011-03-22 16:21:38]

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

[albert.strasheim, 2011-03-22 16:34:28]

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

[rsc, 2011-03-22 16:42:35]

> 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

[albert.strasheim, 2011-03-23 06:02:22]

Hello

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'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

[albert.strasheim, 2011-03-23 06:44:41]

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

[brainman, 2011-03-23 06:45:31]

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

[rsc, 2011-03-23 07:02:23]

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

[albert.strasheim, 2011-03-23 13:36:07]

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

[rsc, 2011-03-23 13:43:46]

> 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, 2011-03-23 14:34:18]

Hello

On Wed, Mar 23, 2011 at 3:43 PM, Russ Cox <rsc@golang.org> wrote:
>> 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.

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

[rsc, 2011-03-23 14:43:40]

> 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

[albert.strasheim, 2011-03-23 15:05:43]

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.

[rsc, 2011-03-23 15:15:11]

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#new...
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

[albert.strasheim, 2011-03-23 15:24:51]

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#new...
> 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.

[rsc, 2011-03-25 18:41:20]

LGTM

[rsc, 2011-03-25 18:42:28]

*** Submitted as 88ca3ce546aa ***

net: add FileConn, FilePacketConn, FileListener

R=iant, rsc, brainman
CC=golang-dev
http://codereview.appspot.com/4306042

Committer: Russ Cox <rsc@golang.org>