FAQ
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/

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() {

Search Discussions

  • Brad Fitzpatrick at Dec 19, 2012 at 11:05 pm
    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 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() {

  • Minux at Dec 19, 2012 at 11:23 pm

    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.
    you cannot do that portablely.
    however, if runtime.Caller(1) returns the filename runtime/panic.c
    but recover() == nil,
    you know the user has played a joke on you, and you should play it back. ;)
  • Brad Fitzpatrick at Dec 19, 2012 at 11:28 pm
    On Wed, Dec 19, 2012 at 3:23 PM, minux wrote:
    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.
    you cannot do that portablely.
    however, if runtime.Caller(1) returns the filename runtime/panic.c
    but recover() == nil,
    you know the user has played a joke on you, and you should play it back. ;
    )
    Hah, awesome. But no. :-)
  • Andrew Gerrand at Dec 19, 2012 at 11:24 pm
    Evil!

    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 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() {

  • Adg at Dec 19, 2012 at 11:24 pm
  • Bradfitz at Dec 19, 2012 at 11:39 pm
    *** 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/
  • Dave Cheney at Dec 19, 2012 at 11:42 pm
    blast, I was too slow. Please consider this as a follow up

    https://codereview.appspot.com/6970049
    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/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedDec 19, '12 at 10:56p
activeDec 19, '12 at 11:42p
posts8
users4
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase