code review 4953047: net: return error from Accept when Listener is closed

net: return error from Accept when Listener is closed

Fixes issue 1898

[jmhodges, 2011-08-26 09:43:21]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2011-08-26 09:50:57]

http://codereview.appspot.com/4953047/diff/5001/src/pkg/net/net_test.go
File src/pkg/net/net_test.go (right):

http://codereview.appspot.com/4953047/diff/5001/src/pkg/net/net_test.go#newco...
src/pkg/net/net_test.go:131: runtime.Gosched()
Rather than a Gosched, I'd probably run the Accept in a goroutine too, with both
the Acceptor goroutine and the Closer goroutine waiting first on their own
unbuffered channels, then your main test goroutine can:

acceptc <- true
closec <- true

*shrug*

Happy to see this fixed, though!

[jmhodges, 2011-08-26 10:43:12]

Two things:

1) I can't quite figure out how to get an EAGAIN in the Write and Read
paths given the net API and I'm pretty sure they also need to be
tested (esp. *netfd.Write, since it means adding  `fd.cw <- true` to
Close()). Any ideas?

2) Adding those channels to this test doubles the code size and
requires the addition of a third channel to signal that the test
passed. Otherwise, the Accept goroutine dies off and the tests always
"pass". I'm not sure it's a win, but I recognize that futzing with the
scheduler is less good than using the primitives of the language. I'll
defer to y'all if you say it's better:

func TestCloseCausesAcceptError(t *testing.T) {
	acceptc := make(chan bool)
	closec := make(chan bool)
	okayc := make(chan bool)
	l, err := Listen("tcp", "localhost:0")
	if err != nil {
		t.Fatal(err)
	}
	go func(ll Listener) {
		<-closec
		ll.Close()
	}(l)
	go func(ll Listener) {
		<-acceptc
		_, err = ll.Accept()
		if err != os.EINVAL {
			t.Errorf("expected %q, got %q", os.EINVAL, err)
		}
		// necessary else this goroutine simply dies off and the test always
		// "passes"
		okayc <- true
	}(l)
	acceptc <- true
	closec <- true
	<-okayc
}

On Fri, Aug 26, 2011 at 2:50 AM,  <bradfitz@golang.org> wrote:
>
> http://codereview.appspot.com/4953047/diff/5001/src/pkg/net/net_test.go
> File src/pkg/net/net_test.go (right):
>
>
http://codereview.appspot.com/4953047/diff/5001/src/pkg/net/net_test.go#newco...
> src/pkg/net/net_test.go:131: runtime.Gosched()
> Rather than a Gosched, I'd probably run the Accept in a goroutine too,
> with both the Acceptor goroutine and the Closer goroutine waiting first
> on their own unbuffered channels, then your main test goroutine can:
>
> acceptc <- true
> closec <- true
>
> *shrug*
>
> Happy to see this fixed, though!
>
> http://codereview.appspot.com/4953047/
>

[bradfitz, 2011-08-26 10:53:30]

On Fri, Aug 26, 2011 at 2:42 PM, Jeff Hodges <jeff@somethingsimilar.com>wrote:

>
> 2) Adding those channels to this test doubles the code size and
> requires the addition of a third channel to signal that the test
> passed. Otherwise, the Accept goroutine dies off and the tests always
> "pass". I'm not sure it's a win, but I recognize that futzing with the
> scheduler is less good than using the primitives of the language. I'll
> defer to y'all if you say it's better:
>
> func TestCloseCausesAcceptError(t *testing.T) {
>         acceptc := make(chan bool)
>        closec := make(chan bool)
>        okayc := make(chan bool)
>

Those three could go on one line


>         l, err := Listen("tcp", "localhost:0")
>        if err != nil {
>                t.Fatal(err)
>        }
>        go func(ll Listener) {
>

no need to take a listener.  it's a closure... just use "l" directly.  it's
guaranteed to be initialized when you need it below.


>                 <-closec
>                ll.Close()
>        }(l)
>        go func(ll Listener) {
>

likewise


>                <-acceptc
>                _, err = ll.Accept()
>                 if err != os.EINVAL {
>                        t.Errorf("expected %q, got %q", os.EINVAL, err)
>                }
>                 // necessary else this goroutine simply dies off and the
> test always
>                // "passes"
>                okayc <- true
>        }(l)
>        acceptc <- true
>        closec <- true
>        <-okayc
>

I like this version more, despite the extra length.

This also means that you can switch your final <-okayc into:

select {
case <-donec:
case <-time.After(5e9):
   t.Fatalf("Accept hung")
}

Then when the test does fail on some operating system (inevitably), we get a
good error message, not an actual test hang.

[rsc, 2011-08-26 20:49:03]

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

http://codereview.appspot.com/4953047/diff/5001/src/pkg/net/fd.go#newcode356
src/pkg/net/fd.go:356: fd.cr <- true
I don't think this code can just fiddle with fd.cr here.
The poll server tracks the number of pending reads
and is careful to issue only that many sends on fd.cr.
Otherwise you might get blocked here if you're out of sync.
This should call into the poll server to record the close.
Honestly I am a bit surprised that a Shutdown on fd.sysfd
does not wake up epoll already.

[jmhodges, 2011-08-28 00:18:13]

On Fri, Aug 26, 2011 at 1:49 PM,  <rsc@golang.org> wrote:
>
> http://codereview.appspot.com/4953047/diff/5001/src/pkg/net/fd.go
> File src/pkg/net/fd.go (right):
>
> http://codereview.appspot.com/4953047/diff/5001/src/pkg/net/fd.go#newcode356
> src/pkg/net/fd.go:356: fd.cr <- true
> I don't think this code can just fiddle with fd.cr here.
> The poll server tracks the number of pending reads
> and is careful to issue only that many sends on fd.cr.
> Otherwise you might get blocked here if you're out of sync.
> This should call into the poll server to record the close.
> Honestly I am a bit surprised that a Shutdown on fd.sysfd
> does not wake up epoll already.
>
> http://codereview.appspot.com/4953047/
>

It doesn't and neither does kevent. I've cleaned up the test code, but
there is much more work needed to be done. The way I hear others try
to do this is set a timeout on them and try to read from the socket to
detect closure. I'm still fumbling through and talking to people about
it. Ideas welcome.

[jmhodges, 2011-09-12 00:58:54]

*** Abandoned ***