code review 12727043: go.net: add netutil package with LimitListener

go.net: add netutil package with LimitListener

Update issue 6012

[adg, 2013-08-12 00:16:33]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go.net

[dsymonds, 2013-08-12 00:36:00]

cute. I don't have an opinion on whether this should be in go.net, but here's a
couple of little notes.

https://codereview.appspot.com/12727043/diff/6001/netutil/listen.go
File netutil/listen.go (right):

https://codereview.appspot.com/12727043/diff/6001/netutil/listen.go#newcode37
netutil/listen.go:37: ch chan struct{}
this could have a direction annotation.

https://codereview.appspot.com/12727043/diff/6001/netutil/listen_test.go
File netutil/listen_test.go (right):

https://codereview.appspot.com/12727043/diff/6001/netutil/listen_test.go#newc...
netutil/listen_test.go:32: go http.Serve(l, http.HandlerFunc(func(w
http.ResponseWriter, r *http.Request) {
use net/http/httptest?

[adg, 2013-08-12 00:44:32]

https://codereview.appspot.com/12727043/diff/6001/netutil/listen.go
File netutil/listen.go (right):

https://codereview.appspot.com/12727043/diff/6001/netutil/listen.go#newcode37
netutil/listen.go:37: ch chan struct{}
On 2013/08/12 00:36:00, dsymonds wrote:
> this could have a direction annotation.

Done.

https://codereview.appspot.com/12727043/diff/6001/netutil/listen.go#newcode37
netutil/listen.go:37: ch chan struct{}
On 2013/08/12 00:36:00, dsymonds wrote:
> this could have a direction annotation.

Done.

https://codereview.appspot.com/12727043/diff/6001/netutil/listen_test.go
File netutil/listen_test.go (right):

https://codereview.appspot.com/12727043/diff/6001/netutil/listen_test.go#newc...
netutil/listen_test.go:32: go http.Serve(l, http.HandlerFunc(func(w
http.ResponseWriter, r *http.Request) {
On 2013/08/12 00:36:00, dsymonds wrote:
> use net/http/httptest?

One of the key benefits of httptest is it sets up the listener and gives you a
server address, but that's exactly what I need to test and demonstrate here.

[dsymonds, 2013-08-12 00:50:42]

https://codereview.appspot.com/12727043/diff/6001/netutil/listen_test.go
File netutil/listen_test.go (right):

https://codereview.appspot.com/12727043/diff/6001/netutil/listen_test.go#newc...
netutil/listen_test.go:32: go http.Serve(l, http.HandlerFunc(func(w
http.ResponseWriter, r *http.Request) {
On 2013/08/12 00:44:32, adg wrote:
> On 2013/08/12 00:36:00, dsymonds wrote:
> > use net/http/httptest?
> 
> One of the key benefits of httptest is it sets up the listener and gives you a
> server address, but that's exactly what I need to test and demonstrate here.

httptest.Server exposes the Listener for exactly this reason.


srv := httptest.NewUnstartedServer(http.HandlerFunc(...) {
  ...
}
srv.Listener = LimitListener(srv.Listener, max)
defer srv.Close()

[adg, 2013-08-12 00:52:12]

On 12 August 2013 10:50, <dsymonds@golang.org> wrote:

> httptest.Server exposes the Listener for exactly this reason.
>
>
> srv := httptest.NewUnstartedServer(**http.HandlerFunc(...) {
>   ...
> }
> srv.Listener = LimitListener(srv.Listener, max)
> defer srv.Close()
>

Sure, but I want programmers to be able to look at the test and see how
they should use it. httptest really doesn't save us much here, and makes
the code a lot less obvious.

[rsc, 2013-08-13 17:40:25]

LGTM

https://codereview.appspot.com/12727043/diff/7003/netutil/listen.go
File netutil/listen.go (right):

https://codereview.appspot.com/12727043/diff/7003/netutil/listen.go#newcode25
netutil/listen.go:25: 
add a Close method on limitListener that will l.Listener.Close(); close(ch).
then l.Close unblocks any pending Accepts, which will then return errors.

[adg, 2013-08-14 01:00:11]

*** Submitted as
https://code.google.com/p/go/source/detail?r=ac736dc34602&repo=net ***

go.net: add netutil package with LimitListener

Update issue 6012

R=golang-dev, dsymonds, rsc
CC=golang-dev
https://codereview.appspot.com/12727043

[ngaut1, 2013-08-14 02:28:54]

i think we should check if error is nil, programmers may use two goroutines to
hanle a connection, one for reading, the other for writing, so Close() may be
called twice when there is an error on connection.

https://codereview.appspot.com/12727043/diff/20001/netutil/listen.go
File netutil/listen.go (right):

https://codereview.appspot.com/12727043/diff/20001/netutil/listen.go#newcode42
netutil/listen.go:42: l.ch <- struct{}{}
i think we should check if error is nil, programmers may use two goroutines to
hanle a connection, one for reading, the other for writing, so Close() may be
called twice when there is an error on connection.

if err == nil {
	l.ch <- struct{}{}
}

[rog, 2013-08-14 08:02:09]

I think ngautl has a good point about the Close logic - it's not uncommon to
close a connection while another goroutine is operating on it.

https://codereview.appspot.com/12727043/diff/20001/netutil/listen.go
File netutil/listen.go (right):

https://codereview.appspot.com/12727043/diff/20001/netutil/listen.go#newcode15
netutil/listen.go:15: for i := 0; i < n; i++ {
I don't think you actually need to fill
the channel here - we're not using the
channel as a memory barrier here -
I believe it'll work just as effectively
for this purpose if you start empty and
fill it up.

This may make a difference for if the maximum
number of connections is quite high.

https://codereview.appspot.com/12727043/diff/20001/netutil/listen.go#newcode42
netutil/listen.go:42: l.ch <- struct{}{}
On 2013/08/14 02:28:54, ngaut1 wrote:
> i think we should check if error is nil, programmers may use two goroutines to
> hanle a connection, one for reading, the other for writing, so Close() may be
> called twice when there is an error on connection.
> 
> if err == nil {
> 	l.ch <- struct{}{}
> }

+1 to the sentiment, but I'm not sure that's quite right - we're not guaranteed
to get a nil error when closing the connection for the first time AFAIK.

I'd put some sync logic around the channel op to make sure
it can only be called once.

For instance:

type limitListenerConn struct {
    net.Conn
    ch chan<-struct{}
    closeReported uint32
}

func (l *limitListenerConn) Close() error { 
   err := l.Conn.Close(0
   if sync.CompareAndSwapUint32(&l.closeReported, 0, 1) {
       l.ch <- struct{}{}
   }
   return err
}

Or use sync.Once or sync.Mutex to taste.

[adg, 2013-08-15 01:19:57]

https://codereview.appspot.com/12727043/diff/20001/netutil/listen.go
File netutil/listen.go (right):

https://codereview.appspot.com/12727043/diff/20001/netutil/listen.go#newcode15
netutil/listen.go:15: for i := 0; i < n; i++ {
On 2013/08/14 08:02:09, rog wrote:
> I don't think you actually need to fill
> the channel here - we're not using the
> channel as a memory barrier here -
> I believe it'll work just as effectively
> for this purpose if you start empty and
> fill it up.
> 
> This may make a difference for if the maximum
> number of connections is quite high.

The allocation of the buffer is the expensive part. Filling it up should be a
secondary concern.

To me it is clearer to put "connection tokens" in the buffer and to "give" one
to each active connection.

Let's worry about efficiency if it actually affects someone.

https://codereview.appspot.com/12727043/diff/20001/netutil/listen.go#newcode42
netutil/listen.go:42: l.ch <- struct{}{}
Thanks for the suggestion guys.

I went with sync.Once: https://codereview.appspot.com/12967043/