code review 85560045: net/http: close Body in client code always, even on err...

net/http: close Body in client code always, even on errors, and document

Fixes Issue 6981

[bradfitz, 2014-04-11 06:25:01]

Hello golang-codereviews@googlegroups.com (cc: adg@golang.org,
dsymonds@golang.org, rsc@golang.org),

I'd like you to review this change to
https://go.googlecode.com/hg/

[ioe, 2014-04-12 21:44:01]

On Friday, April 11, 2014 8:25:02 AM UTC+2, Brad Fitzpatrick wrote:
> net/http: close Body in client code always, even on errors, and document

How will working code behave, that closed the body on error?

While this behavior is much nicer, it might break existing users, since
io.Closer doesn't specify how double closes should behave. In practise they will
usually return an error or worse, if they are not a NopCloser.

[bradfitz, 2014-04-12 21:54:10]

On Sat, Apr 12, 2014 at 2:44 PM, <nightlyone@googlemail.com> wrote:

> On Friday, April 11, 2014 8:25:02 AM UTC+2, Brad Fitzpatrick wrote:
> > net/http: close Body in client code always, even on errors, and document
>
> How will working code behave, that closed the body on error?
>
> While this behavior is much nicer, it might break existing users, since
> io.Closer doesn't specify how double closes should behave. In practise they
> will usually return an error or worse, if they are not a NopCloser.


It's easy to think that before you would get exactly 0 or exactly 1 close
(and that's true).  But it's false that you could determine whether it was
0 or 1 based on the return value from RoundTrip (as documented in 1 place
on Post before but not completely true).  There were too many error paths
that would still return an error from RoundTrip but close the body after it
was written.

Anybody who cared already about closing exactly once and also not leaking
(if there was something to leak on a forgotten close) would've had to wrap
their Body ReadCloser with a Closer implementation that only closed once,
and defensively re-closed it after the HTTP request anyway, since the old
behavior was random with regard to when it closed.

So I assert that anybody sensitive was already being cautious.

That leaves people who didn't care, and this CL is safer and more
consistent by default.

Besides, in my experience, most io.Closer implementations return nil on
double-close.

[ioe, 2014-04-13 13:41:33]

Thanks for this great explanation detailing your research for this issue.

Does it make sense to add something like this explanation to the Go 1.3
changes?


On Saturday, April 12, 2014 11:54:08 PM UTC+2, Brad Fitzpatrick wrote:
> On Sat, Apr 12, 2014 at 2:44 PM,  <night...@googlemail.com> wrote:
> 
> 
> On Friday, April 11, 2014 8:25:02 AM UTC+2, Brad Fitzpatrick wrote:
> 
> 
> > net/http: close Body in client code always, even on errors, and document
> 
> 
> 
> How will working code behave, that closed the body on error?
> 
> 
> 
> While this behavior is much nicer, it might break existing users, since
io.Closer doesn't specify how double closes should behave. In practise they will
usually return an error or worse, if they are not a NopCloser.
> 
> 
> 
> 
> It's easy to think that before you would get exactly 0 or exactly 1 close (and
that's true).  But it's false that you could determine whether it was 0 or 1
based on the return value from RoundTrip (as documented in 1 place on Post
before but not completely true).  There were too many error paths that would
still return an error from RoundTrip but close the body after it was written.
> 
> 
> 
> Anybody who cared already about closing exactly once and also not leaking (if
there was something to leak on a forgotten close) would've had to wrap their
Body ReadCloser with a Closer implementation that only closed once, and
defensively re-closed it after the HTTP request anyway, since the old behavior
was random with regard to when it closed.
> 
> 
> 
> 
> So I assert that anybody sensitive was already being cautious.
> 
> 
> That leaves people who didn't care, and this CL is safer and more consistent
by default.
> 
> 
> 
> 
> Besides, in my experience, most io.Closer implementations return nil on
double-close.

[bradfitz, 2014-04-14 14:47:50]

Ping. Russ? Would like to get this in before beta.
 On Apr 12, 2014 2:54 PM, "Brad Fitzpatrick" <bradfitz@golang.org> wrote:

> On Sat, Apr 12, 2014 at 2:44 PM, <nightlyone@googlemail.com> wrote:
>
>> On Friday, April 11, 2014 8:25:02 AM UTC+2, Brad Fitzpatrick wrote:
>> > net/http: close Body in client code always, even on errors, and document
>>
>> How will working code behave, that closed the body on error?
>>
>> While this behavior is much nicer, it might break existing users, since
>> io.Closer doesn't specify how double closes should behave. In practise they
>> will usually return an error or worse, if they are not a NopCloser.
>
>
> It's easy to think that before you would get exactly 0 or exactly 1 close
> (and that's true).  But it's false that you could determine whether it was
> 0 or 1 based on the return value from RoundTrip (as documented in 1 place
> on Post before but not completely true).  There were too many error paths
> that would still return an error from RoundTrip but close the body after it
> was written.
>
> Anybody who cared already about closing exactly once and also not leaking
> (if there was something to leak on a forgotten close) would've had to wrap
> their Body ReadCloser with a Closer implementation that only closed once,
> and defensively re-closed it after the HTTP request anyway, since the old
> behavior was random with regard to when it closed.
>
> So I assert that anybody sensitive was already being cautious.
>
> That leaves people who didn't care, and this CL is safer and more
> consistent by default.
>
> Besides, in my experience, most io.Closer implementations return nil on
> double-close.
>
>

[rsc, 2014-04-14 14:51:00]

LGTM

let's add something to the 1.3 notes too

[bradfitz, 2014-04-14 15:06:18]

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

net/http: close Body in client code always, even on errors, and document

Fixes Issue 6981

LGTM=rsc
R=golang-codereviews, nightlyone
CC=adg, dsymonds, golang-codereviews, rsc
https://codereview.appspot.com/85560045

[gobot, 2014-04-14 15:26:43]

This CL appears to have broken the linux-amd64-race builder.
See http://build.golang.org/log/a2e401fdcd4903a61a3375bff5da702a20ddafad

[bradfitz, 2014-04-14 15:27:51]

Sigh. On it.
On Apr 14, 2014 8:26 AM, <gobot@golang.org> wrote:

> This CL appears to have broken the linux-amd64-race builder.
> See http://build.golang.org/log/a2e401fdcd4903a61a3375bff5da702a20ddafad
>
> https://codereview.appspot.com/85560045/
>

[0intro, 2014-04-14 20:01:11]

It seems TestTransportClosesBodyOnError was sometimes timing out on Plan 9 and
is now always timing out since CL 87640043. I'm investigating it.

[0intro, 2014-04-14 20:59:08]

> It seems TestTransportClosesBodyOnError was sometimes timing out on Plan 9 and
> is now always timing out since CL 87640043. I'm investigating it.

The problem is ioutil.ReadAll(r.Body) never returns.

[bradfitz, 2014-04-14 21:08:31]

On Mon, Apr 14, 2014 at 1:59 PM, <0intro@gmail.com> wrote:

> It seems TestTransportClosesBodyOnError was sometimes timing out on
>>
> Plan 9 and
>
>> is now always timing out since CL 87640043. I'm investigating it.
>>
>
> The problem is ioutil.ReadAll(r.Body) never returns.
>

The peer closes the TCP connection. If it's stuck in a syscall Read on the
TCP connection and the peer closes, does that not wake up the Read on plan9?

You can verify the peer closes it with:

diff -r f8ac12d0acae src/pkg/net/http/transport.go
--- a/src/pkg/net/http/transport.go Mon Apr 14 13:03:03 2014 -0700
+++ b/src/pkg/net/http/transport.go Mon Apr 14 14:07:30 2014 -0700
@@ -1011,6 +1011,7 @@
  for {
  select {
  case err := <-writeErrCh:
+ log.Printf("Write error = %v", err)
  if err != nil {
  re = responseAndError{nil, err}
  pc.close()


And:

$ go test -v -run=TestTransportClosesBodyOnError
=== RUN TestTransportClosesBodyOnError
2014/04/14 14:06:06 Write error = fake error
--- PASS: TestTransportClosesBodyOnError (0.26 seconds)
PASS
ok   net/http 0.293s


Do you not see that?

[0intro, 2014-04-14 21:15:24]

> $ go test -v -run=TestTransportClosesBodyOnError
> === RUN TestTransportClosesBodyOnError
> 2014/04/14 14:06:06 Write error = fake error
> --- PASS: TestTransportClosesBodyOnError (0.26 seconds)
> PASS
> ok   net/http 0.293s
> 
> Do you not see that?

Yes, I get:

cpu% go test -v -run TestTransportClosesBodyOnError
=== RUN TestTransportClosesBodyOnError
2014/04/14 23:13:16 Write error = fake error

-- 
David du Colombier

[bradfitz, 2014-04-14 21:21:04]

On Mon, Apr 14, 2014 at 2:15 PM, David du Colombier <0intro@gmail.com>wrote:

> > $ go test -v -run=TestTransportClosesBodyOnError
> > === RUN TestTransportClosesBodyOnError
> > 2014/04/14 14:06:06 Write error = fake error
> > --- PASS: TestTransportClosesBodyOnError (0.26 seconds)
> > PASS
> > ok   net/http 0.293s
> >
> > Do you not see that?
>
> Yes, I get:
>
> cpu% go test -v -run TestTransportClosesBodyOnError
> === RUN TestTransportClosesBodyOnError
> 2014/04/14 23:13:16 Write error = fake error


What's this produce on plan9?

package main

import (
        "log"
        "net"
        "os"
        "time"
)

func main() {
        ln := newLocalListener()
        go func() {
                c, err := ln.Accept()
                if err != nil {
                        log.Fatal(err)
                }
                buf := make([]byte, 4096)
                n, err := c.Read(buf)
                log.Printf("Read = %d (%q), %v", n, buf[:n], err)
                os.Exit(0)
        }()
        c, err := net.Dial("tcp", ln.Addr().String())
        if err != nil {
                log.Fatal(err)
        }
        time.Sleep(100 * time.Millisecond)
        c.Close()
        select {}
}

func newLocalListener() net.Listener {
        ln, err := net.Listen("tcp", "127.0.0.1:0")
        if err != nil {
                ln, err = net.Listen("tcp6", "[::1]:0")
        }
        if err != nil {
                log.Fatal(err)
        }
        return ln
}

[aram2, 2014-04-14 21:23:24]

On Mon, Apr 14, 2014 at 11:08 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> The peer closes the TCP connection. If it's stuck in a syscall Read on the
> TCP connection and the peer closes, does that not wake up the Read on plan9?

Closing a file does not unblock readers on Plan 9.

-- 
Aram Hăvărneanu

[bradfitz, 2014-04-14 21:26:27]

On Mon, Apr 14, 2014 at 2:23 PM, Aram Hăvărneanu <aram.h@mgk.ro> wrote:

> On Mon, Apr 14, 2014 at 11:08 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > The peer closes the TCP connection. If it's stuck in a syscall Read on
> the
> > TCP connection and the peer closes, does that not wake up the Read on
> plan9?
>
> Closing a file does not unblock readers on Plan 9.
>

That seems like a bug, then.

Regardless of what Plan 9's native behavior is with itself, Go code assumes
the Close does wake up Readers.

Is there a issue number tracking that?

Has anybody yet prepared a Plan 9 VM image for others to use, as Russ asked
for awhile back?

David, please confirm you see my test program above hang forever, or
deadlock.  On my machines it says (0, EOF).

[0intro, 2014-04-14 21:26:37]

> What's this produce on plan9?

cpu% go run brad.go
2014/04/14 23:25:44 Read = 0 (""), EOF

-- 
David du Colombier

[aram2, 2014-04-14 21:29:08]

I have prototype code that uses notes (plan 9 signals) to properly do
net Closing. I'm sorry I haven't had the time to polish it up for this
cycle.

-- 
Aram Hăvărneanu

[aram2, 2014-04-14 21:29:57]

On Mon, Apr 14, 2014 at 11:28 PM, Aram Hăvărneanu <aram.h@mgk.ro> wrote:
> I have prototype code that uses notes (plan 9 signals) to properly do
> net Closing. I'm sorry I haven't had the time to polish it up for this
> cycle.

...and that also fixes all the timeout stuff.

-- 
Aram Hăvărneanu

[0intro, 2014-04-14 21:30:15]

> Is there a issue number tracking that?

I don't think so. I've created the issue 7782 for
the current problem however.

> Has anybody yet prepared a Plan 9 VM image for others to use, as Russ
> asked for awhile back?

Yes, it is available on:

http://www.9legacy.org/download/plan9-go.img.bz2

-- 
David du Colombier

[0intro, 2014-04-14 21:35:06]

> > I have prototype code that uses notes (plan 9 signals) to properly
> > do net Closing. I'm sorry I haven't had the time to polish it up
> > for this cycle.
> 
> ...and that also fixes all the timeout stuff.

By the way, there is issue 7237 to track the missing
timeout implementation.

-- 
David du Colombier

[bradfitz, 2014-04-14 21:37:55]

On Mon, Apr 14, 2014 at 2:34 PM, David du Colombier <0intro@gmail.com>wrote:

> > > I have prototype code that uses notes (plan 9 signals) to properly
> > > do net Closing. I'm sorry I haven't had the time to polish it up
> > > for this cycle.
> >
> > ...and that also fixes all the timeout stuff.
>
> By the way, there is issue 7237 to track the missing
> timeout implementation.


I've created an OS-Plan9 standard label and applied it to all the bugs I
could find:

https://code.google.com/p/go/issues/list?can=2&q=OS%3DPlan9

A couple bugs were already using it.

[0intro, 2014-04-14 21:41:35]

> I've created an OS-Plan9 standard label and applied it to all the
> bugs I could find:
> 
> https://code.google.com/p/go/issues/list?can=2&q=OS%3DPlan9
> 
> A couple bugs were already using it.

Thanks. All the Plan 9 bugs seem to have the label now.

-- 
David du Colombier

[bradfitz, 2014-04-14 22:08:53]

On Mon, Apr 14, 2014 at 2:29 PM, David du Colombier <0intro@gmail.com>wrote:

> > Is there a issue number tracking that?
>
> I don't think so. I've created the issue 7782 for
> the current problem however.
>
> > Has anybody yet prepared a Plan 9 VM image for others to use, as Russ
> > asked for awhile back?
>
> Yes, it is available on:
>
> http://www.9legacy.org/download/plan9-go.img.bz2


I tried to boot it with Virtual Box (after unbzipping it and renaming it
*.qcow, since it looked like it had the qcow magic number) but it said
failure to read storage.

Any tips?

[0intro, 2014-04-15 04:32:20]

> I tried to boot it with Virtual Box (after unbzipping it and renaming
> it *.qcow, since it looked like it had the qcow magic number) but it
> said failure to read storage.

This is a qcow2 image, which is perhaps not supported by VirtualBox.

You can convert a qcow2 image to vdi using qemu-img:

qemu-img convert -f qcow2 plan9-go.img -O vdi plan9-go.vdi

-- 
David du Colombier

[0intro, 2014-04-15 06:11:20]

I'm uploading a vdi image:

http://www.9legacy.org/download/plan9-go.vdi.bz2

You have to use either VirtualBox < 4.2 or >= 4.2.14.

You should be able to use Plan 9 as long as you choose
the "Intel PRO/1000 MT Server" Ethernet controller.

Let me know if it works for you.

-- 
David du Colombier

[0intro, 2014-04-15 22:21:00]

Aram started a change to implement proper net closing and timeout
on Plan 9, which should fix this issue, among other things.
However, it will not get ready until after Go 1.3 release.

What should we do about this issue?

Thanks.

-- 
David du Colombier

[bradfitz, 2014-04-15 22:32:39]

Is there any urgency? We're not releasing Go 1.3 binaries for Plan 9 anyway.

If you want to add a t.Skip for runtime.GOOS == "plan9" for that http test
so it doesn't mask other regressions, that's fine.  As long as the t.Skip
string contains a reference to golang.org/issue/NNNNN.



On Tue, Apr 15, 2014 at 3:20 PM, David du Colombier <0intro@gmail.com>wrote:

> Aram started a change to implement proper net closing and timeout
> on Plan 9, which should fix this issue, among other things.
> However, it will not get ready until after Go 1.3 release.
>
> What should we do about this issue?
>
> Thanks.
>
> --
> David du Colombier
>

[0intro, 2014-04-15 22:46:48]

> Is there any urgency? We're not releasing Go 1.3 binaries for Plan 9
> anyway.
> 
> If you want to add a t.Skip for runtime.GOOS == "plan9" for that http
> test so it doesn't mask other regressions, that's fine.  As long as
> the t.Skip string contains a reference to golang.org/issue/NNNNN.

I'm a bit worried by the fact we can miss other issues when
the builder is always displaying "fail". Also, the others
tests are no longer being run.

I've submitted CL 87800044 to skip this particular test.

-- 
David du Colombier