code review 2904041: net: provide public access to file descriptors

net: provide public access to file descriptors

Fixes issue 918.

[kr, 2010-11-04 19:32:37]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[rsc1, 2010-11-04 19:56:11]

looks great.  please make the comment change below
also please change the CL text to say

Fixes issue 918.

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

http://codereview.appspot.com/2904041/diff/2001/src/pkg/net/tcpsock.go#newcod...
src/pkg/net/tcpsock.go:211: // File returns a copy of the underlying os.File,
set to blocking mode.
please add (to all the comments, unfortunately)

// It is the caller's responsibility to close f when finished.
// Closing c does not affect f, and closing f does not affect c.

[rog, 2010-11-04 20:26:07]

i haven't checked, but i'm not sure this will work correctly
as the call to make the fd blocking will affect the original
fd too; hence calling Conn.File() will poison the original connection.

i'm probably missing something.

On 4 November 2010 19:56,  <rsc@google.com> wrote:
> looks great.  please make the comment change below
> also please change the CL text to say
>
> Fixes issue 918.
>
>
>
> http://codereview.appspot.com/2904041/diff/2001/src/pkg/net/tcpsock.go
> File src/pkg/net/tcpsock.go (right):
>
>
http://codereview.appspot.com/2904041/diff/2001/src/pkg/net/tcpsock.go#newcod...
> src/pkg/net/tcpsock.go:211: // File returns a copy of the underlying
> os.File, set to blocking mode.
> please add (to all the comments, unfortunately)
>
> // It is the caller's responsibility to close f when finished.
> // Closing c does not affect f, and closing f does not affect c.
>
> http://codereview.appspot.com/2904041/
>

[rog, 2010-11-04 23:22:06]

oh i see, i'm missing the fact that it doesn't really
matter if net fds block - it'll just mean that the polling
code won't be used if Conn.File() has been called,
which could use a few more OS threads, but that's
probably inevitable.

sorry for the noise.

On 4 November 2010 20:26, roger peppe <rogpeppe@gmail.com> wrote:
> i haven't checked, but i'm not sure this will work correctly
> as the call to make the fd blocking will affect the original
> fd too; hence calling Conn.File() will poison the original connection.
>
> i'm probably missing something.
>
> On 4 November 2010 19:56,  <rsc@google.com> wrote:
>> looks great.  please make the comment change below
>> also please change the CL text to say
>>
>> Fixes issue 918.
>>
>>
>>
>> http://codereview.appspot.com/2904041/diff/2001/src/pkg/net/tcpsock.go
>> File src/pkg/net/tcpsock.go (right):
>>
>>
http://codereview.appspot.com/2904041/diff/2001/src/pkg/net/tcpsock.go#newcod...
>> src/pkg/net/tcpsock.go:211: // File returns a copy of the underlying
>> os.File, set to blocking mode.
>> please add (to all the comments, unfortunately)
>>
>> // It is the caller's responsibility to close f when finished.
>> // Closing c does not affect f, and closing f does not affect c.
>>
>> http://codereview.appspot.com/2904041/
>>
>

[kr, 2010-11-05 00:41:54]

Hello rsc, rog (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2010-11-05 01:32:53]

Please, implement windows version of dup() too (in fd_windows.go). It doesn't
have to do anything exciting (return "Not implemented" should do for now), and
put comment with TODO so we remember about it for later. Thank you.

Alex

[kr, 2010-11-05 02:02:08]

Hello rsc, rog, brainman (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2010-11-05 02:05:01]

On Thu, Nov 4, 2010 at 19:22, roger peppe <rogpeppe@gmail.com> wrote:
> oh i see, i'm missing the fact that it doesn't really
> matter if net fds block - it'll just mean that the polling
> code won't be used if Conn.File() has been called,
> which could use a few more OS threads, but that's
> probably inevitable.
>
> sorry for the noise.

the noise was very interesting.  i read the
man page and it sounded like F_SETFL is
setting fd flags, not inode/socket flags.
of course that is probably a lie, but it
sounded like the existing code was fine.

i agree that if the non-blocking is a socket
property rather than an fd property, then
the fallback that the net.Conn just stops
using epoll is probably the best we're going to do.

russ

[kr, 2010-11-05 02:05:07]

Oops, I didn't know I had to tell hg change about the new file explicitly. Let
me try again.

[kr, 2010-11-05 02:05:18]

Hello rsc, rog, brainman (cc: golang-dev@googlegroups.com),

Please take another look.

[kr, 2010-11-05 02:11:58]

On Thu, Nov 4, 2010 at 7:04 PM, Russ Cox <rsc@golang.org> wrote:
> i agree that if the non-blocking is a socket
> property rather than an fd property, then
> the fallback that the net.Conn just stops
> using epoll is probably the best we're going to do.

On my laptop (Ubuntu), man fcntl seems to confirm
that this is a socket property. It distinguishes "file
status flags" (including O_NONBLOCK) from "file
descriptor flags":

> Duplicated file descriptors (made with dup(2),
> fcntl(F_DUPFD), fork(2), etc.) refer to the same
> open file description, and thus share the same
> file status flags.

In this case, is it reasonable to return the original fd
rather than using dup?

kr

[rsc, 2010-11-05 02:17:35]

> On my laptop (Ubuntu), man fcntl seems to confirm
> that this is a socket property. It distinguishes "file
> status flags" (including O_NONBLOCK) from "file
> descriptor flags":
>
>> Duplicated file descriptors (made with dup(2),
>> fcntl(F_DUPFD), fork(2), etc.) refer to the same
>> open file description, and thus share the same
>> file status flags.
>
> In this case, is it reasonable to return the original fd
> rather than using dup?

i'd rather still use dup, so that Close on the two
different objects doesn't have to be coordinated.

if you share the fd, then you have to worry about
someone doing Close on one (say the net.Conn)
and then getting a new fd via Open that happens
to reuse the same fd, and then doing Close on the
other (say the *os.File) and accidentally closing
the new fd.

russ

[brainman, 2010-11-05 02:21:48]

LGTM

[rog, 2010-11-05 09:27:16]

On 5 November 2010 02:04, Russ Cox <rsc@golang.org> wrote:
> On Thu, Nov 4, 2010 at 19:22, roger peppe <rogpeppe@gmail.com> wrote:
>> oh i see, i'm missing the fact that it doesn't really
>> matter if net fds block - it'll just mean that the polling
>> code won't be used if Conn.File() has been called,
>> which could use a few more OS threads, but that's
>> probably inevitable.
>>
>> sorry for the noise.
>
> the noise was very interesting.  i read the
> man page and it sounded like F_SETFL is
> setting fd flags, not inode/socket flags.
> of course that is probably a lie, but it
> sounded like the existing code was fine.

as is traditional in all unix-like systems that i know of, dup just
makes a new reference to the underlying kernel file structure - a
process's file table is just a collection of pointers, so there is no
place to put per-fd flags - the flags must live in the underlying
structure, which is shared after dup.

dup(2) says "The object referenced by the descriptor does not
distinguish between fildes and fildes2 in any way.  Thus if fildes2
and fildes are duplicate references to an open file, read(2), write(2)
and lseek(2) calls all move a single pointer into the file, and append
mode, non-blocking I/O and asynchronous I/O options are shared between
the references."

you knew this really :-)

one interesting thing is that calling Conn.File() on
a network connection might have the side effect of speeding up
IO, as the code path won't go through the somewhat intricate
polling code. performance critical programs which use relatively
few connections may wish to try it.

[kr, 2010-11-05 17:50:16]

On 2010/11/05 02:17:35, rsc wrote:
> i'd rather still use dup, so that Close on the two
> different objects doesn't have to be coordinated.

That makes sense. Then I think Patch Set 5 is
ready for another look.

kr

[rsc1, 2010-11-05 18:00:17]

LGTM

[rsc, 2010-11-05 18:02:05]

*** Submitted as a8ffe111d8d3 ***

net: provide public access to file descriptors

Fixes issue 918.

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

Committer: Russ Cox <rsc@golang.org>