Unnecessary Shutdown in netFD.Close?

[Albert Strasheim]

Hello all

We are implementing a server that forks a new child process after
accepting a connection, passing along the file descriptor in the fd
slice of os.ForkExec/StartProcess. The child process runs as the uid/
gid of the user that has logged in, like an SSH server.

We will submit the CL for reconstituting a net.Conn from an os.File
soon.

However, there is a syscall.Shutdown in netFD.Close that is breaking
this pattern.

The problem is that if the server calls net.Conn.Close on the socket
after starting the client process, it also shuts down the client's
socket, instead of just closing its file descriptor for the socket,
leaving the client with the other open file descriptor for the socket.

The patch looks like this:

diff -r d1ffd1479d88 src/pkg/net/fd.go
--- a/src/pkg/net/fd.go Mon Feb 07 21:40:33 2011 -0500
+++ b/src/pkg/net/fd.go Tue Feb 08 14:38:07 2011 +0000
@@ -80,9 +80,9 @@
// might help batch requests.
//
// To avoid races in closing, all fd operations are locked and
-// refcounted. when netFD.Close() is called, it calls
syscall.Shutdown
-// and sets a closing flag. Only when the last reference is removed
-// will the fd be closed.
+// refcounted. when netFD.Close() is called, it sets a closing
+// flag. Only when the last reference is removed will the fd be
+// closed.

type pollServer struct {
cr, cw chan *netFD // buffered >= 1
@@ -329,7 +329,6 @@
}

fd.incref()
- syscall.Shutdown(fd.sysfd, syscall.SHUT_RDWR)
fd.closing = true
fd.decref()
return nil
diff -r d1ffd1479d88 src/pkg/net/fd_windows.go
--- a/src/pkg/net/fd_windows.go Mon Feb 07 21:40:33 2011 -0500
+++ b/src/pkg/net/fd_windows.go Tue Feb 08 14:38:07 2011 +0000
@@ -188,7 +188,6 @@
}

fd.incref()
- syscall.Shutdown(fd.sysfd, syscall.SHUT_RDWR)
fd.closing = true
fd.decref()
return nil

All the tests in the net package pass without the Shutdown, so it
doesn't seem like it does anything useful.

Any thoughts or are we on the right track here?

Regards,

Albert

[Russ Cox]

http://groups.google.com/group/golang-nuts/t/5611b0a4894d5d5f

[Albert Strasheim]

Hello

On Feb 8, 5:27 pm, Russ Cox <r...@golang.org> wrote:
> http://groups.google.com/group/golang-nuts/t/5611b0a4894d5d5f

Thanks. Does you or Ian have any new insights about what the right way
to go about this is?

Cheers

Albert

[Ian Lance Taylor]

Albert Strasheim <ful...@gmail.com> writes:

I don't have anything to add to my earlier note: it's probably fine to
skip the Shutdown and check fd.closing in Read, etc. That might require
using two different closing flags, one for read and one for write, and
having Close use rio.Lock and wio.Lock before setting the appropriate
flags.

Ian

[Russ Cox]

It's fine to remove Shutdown as long as you manage to
(1) keep from closing a file descriptor until there is
no chance of any goroutines using it in a system call,
(2) make sure that Close causes any blocked reads
and writes to wake up and return whatever error it is
that they return.

Shutdown was the easiest way to ensure both of those
but it may be possible to do without it. If you can, great!

Russ

[Albert Strasheim]

Hello all

I've run into a new issue with Shutdown.

Is Shutdown also necessary on listen sockets?

Closing a listen socket should presumably cause any accepts on that
socket to unblock? Is Shutdown necessary for that?

We've managed to avoid passing client sockets between Go processes,
but Go also calls Shutdown on listen sockets, which causes some
discomfort for systemd, because it didn't expect services to call
shutdown on the sockets it passes in:

http://lists.freedesktop.org/archives/systemd-devel/2011-April/002084.html

Maybe we should just avoid calling Shutdown on listen sockets as a
first step? I can submit a CL if that makes sense.

Regards

Albert

[Russ Cox]

I would rather see a more general fix with the
properties that I outlined in my last mail.

[Ian Lance Taylor]

Albert Strasheim <ful...@gmail.com> writes:

> On Feb 9, 5:28 am, Russ Cox <r...@golang.org> wrote:
>> It's fine to remove Shutdown as long as you manage to
>> (1) keep from closing a file descriptor until there is
>> no chance of any goroutines using it in a system call,
>> (2) make sure that Close causes any blocked reads
>> and writes to wake up and return whatever error it is
>> that they return.
>> Shutdown was the easiest way to ensure both of those
>> but it may be possible to do without it.  If you can, great!
>
> Is Shutdown also necessary on listen sockets?
>
> Closing a listen socket should presumably cause any accepts on that
> socket to unblock? Is Shutdown necessary for that?

Go code doesn't block on the accept system call, so the question is
whether closing a socket will cause it to be marked as available for
read. That is Russ's question 2 above. On GNU/Linux using epoll an
EPOLLHUP event should occur, which should indeed cause the goroutine to
wake up, and the call to Accept will most likely fail with an EBADF
error indication.

Ian