qbit/go
1
0
Fork 0
mirror of https://github.com/golang/go synced 2024-09-29 11:24:28 -06:00
Code Releases Activity

net/http/httputil: fix goroutine leak for DumpRequestOut

Browse Source 
When an invalid URL was passed to DumpRequestOut, it would directly return
without gracefully shutting down the reader goroutine.

So we create a channel and signal the reader goroutine to exit
if an error occurs during roundtrip.

Fixes #32571

Change-Id: I8c2970f1601e599f3d1ebfed298faf5f5716fc2c
Reviewed-on: https://go-review.googlesource.com/c/go/+/182037
Run-TryBot: Agniva De Sarker <agniva.quicksilver@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
This commit is contained in:
Agniva De Sarker 2019-06-13 10:29:39 +05:30 committed by Agniva De Sarker
parent 8fedb2d338
commit e7a4ab427d
2 changed files with 30 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
src/net/http/httputil/dump.go
View File

@ -111,6 +111,10 @@ func DumpRequestOut(req *http.Request, body bool) ([]byte, error) {
}
defer t.CloseIdleConnections()
// We need this channel to ensure that the reader
// goroutine exits if t.RoundTrip returns an error.
// See golang.org/issue/32571.
quitReadCh := make(chan struct{})
// Wait for the request before replying with a dummy response:
go func() {
req, err := http.ReadRequest(bufio.NewReader(pr))
@ -120,13 +124,18 @@ func DumpRequestOut(req *http.Request, body bool) ([]byte, error) {
io.Copy(ioutil.Discard, req.Body)
req.Body.Close()
}
dr.c <- strings.NewReader("HTTP/1.1 204 No Content\r\nConnection: close\r\n\r\n")
select {
case dr.c <- strings.NewReader("HTTP/1.1 204 No Content\r\nConnection: close\r\n\r\n"):
case <-quitReadCh:
}
}()
_, err := t.RoundTrip(reqSend)
req.Body = save
if err != nil {
pw.Close()
quitReadCh <- struct{}{}
return nil, err
}
dump := buf.Bytes()

20
src/net/http/httputil/dump_test.go
View File

@ -26,6 +26,7 @@ type dumpTest struct {
WantDump string
WantDumpOut string
MustError bool // if true, the test is expected to throw an error
NoBody bool // if true, set DumpRequest{,Out} body to false
}
@ -206,6 +207,16 @@ var dumpTests = []dumpTest{
}
func TestDumpRequest(t *testing.T) {
// Make a copy of dumpTests and add 10 new cases with an empty URL
// to test that no goroutines are leaked. See golang.org/issue/32571.
// 10 seems to be a decent number which always triggers the failure.
dumpTests := dumpTests[:]
for i := 0; i < 10; i++ {
dumpTests = append(dumpTests, dumpTest{
Req: mustNewRequest("GET", "", nil),
MustError: true,
})
}
numg0 := runtime.NumGoroutine()
for i, tt := range dumpTests {
if tt.Req != nil && tt.GetReq != nil || tt.Req == nil && tt.GetReq == nil {
@ -250,6 +261,15 @@ func TestDumpRequest(t *testing.T) {
}
}
if tt.MustError {
req := freshReq(tt)
_, err := DumpRequestOut(req, !tt.NoBody)
if err == nil {
t.Errorf("DumpRequestOut #%d: expected an error, got nil", i)
}
continue
}
if tt.WantDumpOut != "" {
req := freshReq(tt)
dump, err := DumpRequestOut(req, !tt.NoBody)