code review 6867050: net/http: implement CloseNotifier

net/http: implement CloseNotifier

Fixes issue 2510

[bradfitz, 2012-12-03 21:59:24]

Hello golang-dev@googlegroups.com,

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

[rsc, 2012-12-04 15:30:31]

https://codereview.appspot.com/6867050/diff/4001/src/pkg/net/http/server.go
File src/pkg/net/http/server.go (right):

https://codereview.appspot.com/6867050/diff/4001/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:102: // Closed returns a channel which is closed when
the client
Can we make it a chan bool that gets a single true sent instead?
I'm just having trouble with the multiple meanings of close here.
Should probably also be CloseNotify to match the type name:

// CloseNotify returns a channel that will receive true when the
// client connection has been lost or closed.
CloseNotify() chan bool

[bradfitz, 2012-12-04 15:57:12]

https://codereview.appspot.com/6867050/diff/4001/src/pkg/net/http/server.go
File src/pkg/net/http/server.go (right):

https://codereview.appspot.com/6867050/diff/4001/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:102: // Closed returns a channel which is closed when
the client
On 2012/12/04 15:30:31, rsc wrote:
> Can we make it a chan bool that gets a single true sent instead?

I have no preference either way.  I think the broadcast close is fun, but maybe
too surprising?  And people could do that on their own if they want that.

> I'm just having trouble with the multiple meanings of close here.
> Should probably also be CloseNotify to match the type name:
> 
> // CloseNotify returns a channel that will receive true when the
> // client connection has been lost or closed.
> CloseNotify() chan bool

I used the name you suggested in
http://code.google.com/p/go/issues/detail?id=2510#c18

I also assume a receive-only chan still?

[bradfitz, 2012-12-04 21:25:30]

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

Please take another look.

[rsc, 2012-12-06 03:14:45]

LGTM

https://codereview.appspot.com/6867050/diff/4/src/pkg/net/http/server.go
File src/pkg/net/http/server.go (right):

https://codereview.appspot.com/6867050/diff/4/src/pkg/net/http/server.go#newc...
src/pkg/net/http/server.go:151: c.closeNotifyc = make(chan bool)
make(chan bool, 1) to avoid hanging the sender if no one is receiving.

[bradfitz, 2012-12-06 03:16:38]

https://codereview.appspot.com/6867050/diff/4/src/pkg/net/http/server.go
File src/pkg/net/http/server.go (right):

https://codereview.appspot.com/6867050/diff/4/src/pkg/net/http/server.go#newc...
src/pkg/net/http/server.go:151: c.closeNotifyc = make(chan bool)
On 2012/12/06 03:14:45, rsc wrote:
> make(chan bool, 1) to avoid hanging the sender if no one is receiving.

good catch.  missed that when changing it from chan struct{}.

[bradfitz, 2012-12-06 03:25:44]

*** Submitted as https://code.google.com/p/go/source/detail?r=3292433291b2 ***

net/http: implement CloseNotifier

Fixes issue 2510

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