code review 159780043: net/http/fcgi: Fix resource leaks

net/http/fcgi: Fix resource leaks

Close the pipe for the body of a request when it is aborted and close all pipes when child.serve terminates.
Fixes issue 6934.

[esk, 2014-10-13 02:26:52]

Hello golang-codereviews@googlegroups.com,

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

[bradfitz, 2014-10-27 19:28:40]

https://codereview.appspot.com/159780043/diff/40001/src/net/http/fcgi/child.go
File src/net/http/fcgi/child.go (right):

https://codereview.appspot.com/159780043/diff/40001/src/net/http/fcgi/child.g...
src/net/http/fcgi/child.go:231: println("abort")
This line should be removed too. Looks like ancient debugging that slipped in
and was never caught.

https://codereview.appspot.com/159780043/diff/40001/src/net/http/fcgi/fcgi_te...
File src/net/http/fcgi/fcgi_test.go (right):

https://codereview.appspot.com/159780043/diff/40001/src/net/http/fcgi/fcgi_te...
src/net/http/fcgi/fcgi_test.go:154: type readOnlyConn struct {
what is this type?  seems unnecessary.  the caller can do:

ioutil.NopCloser(strings.NewReader("foo"))  (or bytes.NewReader)

https://codereview.appspot.com/159780043/diff/40001/src/net/http/fcgi/fcgi_te...
src/net/http/fcgi/fcgi_test.go:176: var streamBeginTypeStdin =
bytes.Join([][]byte{
add a comment about what this is

https://codereview.appspot.com/159780043/diff/40001/src/net/http/fcgi/fcgi_te...
src/net/http/fcgi/fcgi_test.go:209: func TestChildServeCleansUp(t *testing.T) {
What is this testing? Add a comment. And the comment should probably reference a
bug.

https://codereview.appspot.com/159780043/diff/40001/src/net/http/fcgi/fcgi_te...
src/net/http/fcgi/fcgi_test.go:210: for _, test := range cleanUpTests {
for i, tt := ...

(test is normally spelled "tt")

https://codereview.appspot.com/159780043/diff/40001/src/net/http/fcgi/fcgi_te...
src/net/http/fcgi/fcgi_test.go:217: ioutil.ReadAll(r.Body)
ignoring the error?

https://codereview.appspot.com/159780043/diff/40001/src/net/http/fcgi/fcgi_te...
src/net/http/fcgi/fcgi_test.go:221: <-done
select {
case <-done:
case <-time.After(5 * time.Second);
   t.Error(f("timeout waiting for test index %d", i)
}

[esk, 2014-11-01 04:02:35]

I've added comments to explain the new test. PTAL.

https://codereview.appspot.com/159780043/diff/40001/src/net/http/fcgi/child.go
File src/net/http/fcgi/child.go (right):

https://codereview.appspot.com/159780043/diff/40001/src/net/http/fcgi/child.g...
src/net/http/fcgi/child.go:231: println("abort")
On 2014/10/27 19:28:39, bradfitz wrote:
> This line should be removed too. Looks like ancient debugging that slipped in
> and was never caught.

Done.

https://codereview.appspot.com/159780043/diff/40001/src/net/http/fcgi/fcgi_te...
File src/net/http/fcgi/fcgi_test.go (right):

https://codereview.appspot.com/159780043/diff/40001/src/net/http/fcgi/fcgi_te...
src/net/http/fcgi/fcgi_test.go:154: type readOnlyConn struct {
On 2014/10/27 19:28:40, bradfitz wrote:
> what is this type?  seems unnecessary.  the caller can do:
> 
> ioutil.NopCloser(strings.NewReader("foo"))  (or bytes.NewReader)

Done.

https://codereview.appspot.com/159780043/diff/40001/src/net/http/fcgi/fcgi_te...
src/net/http/fcgi/fcgi_test.go:176: var streamBeginTypeStdin =
bytes.Join([][]byte{
On 2014/10/27 19:28:40, bradfitz wrote:
> add a comment about what this is

Done.

https://codereview.appspot.com/159780043/diff/40001/src/net/http/fcgi/fcgi_te...
src/net/http/fcgi/fcgi_test.go:209: func TestChildServeCleansUp(t *testing.T) {
On 2014/10/27 19:28:39, bradfitz wrote:
> What is this testing? Add a comment. And the comment should probably reference
a
> bug.

Done.

https://codereview.appspot.com/159780043/diff/40001/src/net/http/fcgi/fcgi_te...
src/net/http/fcgi/fcgi_test.go:210: for _, test := range cleanUpTests {
On 2014/10/27 19:28:40, bradfitz wrote:
> for i, tt := ...
> 
> (test is normally spelled "tt")

Done.

https://codereview.appspot.com/159780043/diff/40001/src/net/http/fcgi/fcgi_te...
src/net/http/fcgi/fcgi_test.go:217: ioutil.ReadAll(r.Body)
On 2014/10/27 19:28:39, bradfitz wrote:
> ignoring the error?

Yes. See new comment.

https://codereview.appspot.com/159780043/diff/40001/src/net/http/fcgi/fcgi_te...
src/net/http/fcgi/fcgi_test.go:221: <-done
On 2014/10/27 19:28:40, bradfitz wrote:
> select {
> case <-done:
> case <-time.After(5 * time.Second);
>    t.Error(f("timeout waiting for test index %d", i)
> }

I've added comments to explain that the test deadlocks on failure. I think
that's better than a timeout because it happens immediately. What do you think?

[bradfitz, 2014-11-12 18:58:11]

Unfortunately I was on vacation for the past two weeks and wasn't able to get to
this before the Go 1.4 freeze. Let's discuss in early December for Go 1.5.

In the meantime users can fork this package if needed.

[gobot, 2014-12-19 05:14:29]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/159780043 is best) in the description in your
new CL.

Thanks very much.

[bradfitz, 2014-12-21 05:07:56]

Evan, could you move this over to Gerrit? I'd like to see this fixed at the
beginning of the Go 1.5 dev cycle, before we get near/past the 3 months of
hacking.