[golang-dev] code review 6971049: net/http: fix server connection leak on Handler's panic(nil) (issue 6971049)

On Wed, Dec 19, 2012 at 2:56 PM, wrote:

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev@googlegroups.com,

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


Description:
net/http: fix server connection leak on Handler's panic(nil)

If a handler did a panic(nil), the connection was never closed.

Fixes issue 4050

Please review this at https://codereview.appspot.**com/6971049/<https://codereview.appspot.com/6971049/>

Affected files:
M src/pkg/net/http/serve_test.go
M src/pkg/net/http/server.go


Index: src/pkg/net/http/serve_test.go
==============================**==============================**=======
--- a/src/pkg/net/http/serve_test.**go
+++ b/src/pkg/net/http/serve_test.**go
@@ -918,15 +918,19 @@
}
}

+func TestHandlerPanicNil(t *testing.T) {
+ testHandlerPanic(t, false, nil)
+}
+
func TestHandlerPanic(t *testing.T) {
- testHandlerPanic(t, false)
+ testHandlerPanic(t, false, "intentional death for testing")
}

func TestHandlerPanicWithHijack(t *testing.T) {
- testHandlerPanic(t, true)
+ testHandlerPanic(t, true, "intentional death for testing")
}

-func testHandlerPanic(t *testing.T, withHijack bool) {
+func testHandlerPanic(t *testing.T, withHijack bool, panicValue
interface{}) {
// Unlike the other tests that set the log output to ioutil.Discard
// to quiet the output, this test uses a pipe. The pipe serves
three
// purposes:
@@ -955,7 +959,7 @@
}
defer rwc.Close()
}
- panic("intentional death for testing")
+ panic(panicValue)
}))
defer ts.Close()

@@ -968,7 +972,7 @@
_, err := pr.Read(buf)
pr.Close()
if err != nil {
- t.Fatal(err)
+ t.Error(err)
}
done <- true
}()
@@ -978,6 +982,10 @@
t.Logf("expected an error")
}

+ if panicValue == nil {
+ return
+ }
+
select {
case <-done:
return
Index: src/pkg/net/http/server.go
==============================**==============================**=======
--- a/src/pkg/net/http/server.go
+++ b/src/pkg/net/http/server.go
@@ -716,6 +716,7 @@
c.rwc.Close()
}
}()
+ defer c.close()

if tlsConn, ok := c.rwc.(*tls.Conn); ok {
if err := tlsConn.Handshake(); err != nil {
@@ -791,7 +792,6 @@
break
}
}
- c.close()
}

func (w *response) sendExpectationFailed() {

On Thursday, December 20, 2012, Brad Fitzpatrick wrote:

TIL you can't tell the difference between a non-panic and a panic(nil) in
a recover() call.

On 20 December 2012 10:05, Brad Fitzpatrick wrote:

TIL you can't tell the difference between a non-panic and a panic(nil) in
a recover() call.

Pro-tip: don't call panic(nil).

On Thu, Dec 20, 2012 at 10:39 AM, wrote:
*** Submitted as
https://code.google.com/p/go/source/detail?r=397a572d7fc9 ***


net/http: fix server connection leak on Handler's panic(nil)

If a handler did a panic(nil), the connection was never closed.

Fixes issue 4050

R=golang-dev, adg
CC=golang-dev
https://codereview.appspot.com/6971049


https://codereview.appspot.com/6971049/