Code review - Issue 6553061: code review 6553061: net/http: use r.Body.Close to close connection during T...https://codereview.appspot.com/2012-09-24T07:33:59+00:00rietveld
Message from unknown
2012-09-24T02:28:25+00:00brainmanurn:md5:397b95b7ef2adbefacc977a6cee706b5
Message from unknown
2012-09-24T02:28:29+00:00brainmanurn:md5:eb250f14afc6064322f9c1bdeb4b8669
Message from unknown
2012-09-24T02:29:29+00:00brainmanurn:md5:57e50a46cdfa0ea7af93343ad0069dc9
Message from alex.brainman@gmail.com
2012-09-24T02:29:35+00:00brainmanurn:md5:4c8ba36db915e7d922fae3ba1118d1f3
Hello golang-dev@googlegroups.com,
I'd like you to review this change to
https://go.googlecode.com/hg/
Message from bradfitz@golang.org
2012-09-24T02:31:36+00:00bradfitzurn:md5:85f95d27a2c2f79271a2ae595118e1b1
LGTM
but leaking 1 fd was enough to make the windows builders flaky?
On Sun, Sep 23, 2012 at 7:29 PM, <alex.brainman@gmail.com> wrote:
> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> net/http: use r.Body.Close to close connection during
> TestServeFileFromCWD
>
> Fixes issue 3917.
>
> Please review this at http://codereview.appspot.com/**6553061/<http://codereview.appspot.com/6553061/>
>
> Affected files:
> M src/pkg/net/http/fs_test.go
>
>
> Index: src/pkg/net/http/fs_test.go
> ==============================**==============================**=======
> --- a/src/pkg/net/http/fs_test.go
> +++ b/src/pkg/net/http/fs_test.go
> @@ -340,11 +340,6 @@
> }
>
> func TestServeFileFromCWD(t *testing.T) {
> - if runtime.GOOS == "windows" {
> - // TODO(brainman): find out why this test is broken
> - t.Logf("Temporarily skipping test on Windows; see
> http://golang.org/issue/3917")
> - return
> - }
> ts := httptest.NewServer(**HandlerFunc(func(w ResponseWriter, r
> *Request) {
> ServeFile(w, r, "fs_test.go")
> }))
> @@ -353,6 +348,7 @@
> if err != nil {
> t.Fatal(err)
> }
> + r.Body.Close()
> if r.StatusCode != 200 {
> t.Fatalf("expected 200 OK, got %s", r.Status)
> }
>
>
>
Message from alex.brainman@gmail.com
2012-09-24T02:46:10+00:00brainmanurn:md5:2553c7794f867b93721a3041c5d37fd1
On 2012/09/24 02:31:36, bradfitz wrote:
>
> but leaking 1 fd was enough to make the windows builders flaky?
>
I had the benefit of looking at the builder, so I can investigate the problem. What is happening is ServeFile never finishes, because it waits for the client to receive the file, but client never does, unless I put r.BodyClose in. I think this test is "flaky", and now I am fixing it.
Alex
Message from unknown
2012-09-24T02:48:13+00:00brainmanurn:md5:3337734303c4b1a46e290aafd581437d
Message from alex.brainman@gmail.com
2012-09-24T02:48:27+00:00brainmanurn:md5:2745a399c6054fa755197953e55ff55d
*** Submitted as http://code.google.com/p/go/source/detail?r=8841bdb36594 ***
net/http: use r.Body.Close to close connection during TestServeFileFromCWD
Fixes issue 3917.
R=golang-dev, bradfitz
CC=golang-dev
http://codereview.appspot.com/6553061
Message from bradfitz@golang.org
2012-09-24T05:37:21+00:00bradfitzurn:md5:9d90b38f45ce10c09e6e5d365354cc4a
On Sun, Sep 23, 2012 at 7:46 PM, <alex.brainman@gmail.com> wrote:
> On 2012/09/24 02:31:36, bradfitz wrote:
>
> but leaking 1 fd was enough to make the windows builders flaky?
>>
>
>
> I had the benefit of looking at the builder, so I can investigate the
> problem. What is happening is ServeFile never finishes, because it waits
> for the client to receive the file, but client never does, unless I put
> r.BodyClose in. I think this test is "flaky", and now I am fixing it.
>
That sounds like an implementation difference between Windows and Unix.
The client closing or not shouldn't affect the server's blocking. I'm
more worried about that.
Message from alex.brainman@gmail.com
2012-09-24T07:33:59+00:00brainmanurn:md5:0964c09ef78cce928a84605c24bdbc24
On 2012/09/24 05:37:21, bradfitz wrote:
>
> That sounds like an implementation difference between Windows and Unix.
I wish I could agree with you here, but I do not see how.
> The client closing or not shouldn't affect the server's blocking. I'm
> more worried about that.
Before my change, the client was not closing connection, the server was supposed to. But server could only get to that line after it has finished writing response body. And writing response body was blocked, because client was not reading it. You say server writes should not be affected by client reads, but they are. Writing 1 byte will probably succeed, 100 bytes will too, but 100 Mbytes would fail. Don't you think? So, it just a matter of where this limit is. And it, obviously, varies in different environments.
Here is to demonstrate original problem:
c:\go\src\pkg\net\http>hg par
changeset: 14126:86c06191bea6
tag: tip
user: Mikio Hara <mikioh.mikioh@gmail.com>
date: Wed Sep 19 07:04:12 2012 +0900
summary: cmd/api: fix signatures like func(x, y, z int)
I have applied this:
diff --git a/src/pkg/net/http/alex.go b/src/pkg/net/http/alex.go
new file mode 100644
--- /dev/null
+++ b/src/pkg/net/http/alex.go
@@ -0,0 +1,60 @@
+package http
+
+import (
+ "fmt"
+ "net"
+ "time"
+)
+
+type ConnSniffer struct {
+ conn net.Conn
+}
+
+func NewConnSniffer(conn net.Conn) *ConnSniffer {
+ return &ConnSniffer{
+ conn: conn,
+ }
+}
+
+func showSome(b []byte) string {
+ if len(b) < 40 {
+ return string(b)
+ }
+ return fmt.Sprintf("%q ... %q", b[:20], b[len(b)-20:])
+}
+
+func (s *ConnSniffer) Read(b []byte) (int, error) {
+ n, err := s.conn.Read(b)
+ fmt.Printf("read n=%d data=[%v] err=%v\n", n, showSome(b[:n]), err)
+ return n, err
+}
+
+func (s *ConnSniffer) Write(b []byte) (int, error) {
+ n, err := s.conn.Write(b)
+ fmt.Printf("written n=%d data=[%v] err=%v\n", n, showSome(b[:n]), err)
+ return n, err
+}
+
+func (s *ConnSniffer) Close() error {
+ return s.conn.Close()
+}
+
+func (s *ConnSniffer) LocalAddr() net.Addr {
+ return s.conn.LocalAddr()
+}
+
+func (s *ConnSniffer) RemoteAddr() net.Addr {
+ return s.conn.RemoteAddr()
+}
+
+func (s *ConnSniffer) SetDeadline(t time.Time) error {
+ return s.conn.SetDeadline(t)
+}
+
+func (s *ConnSniffer) SetReadDeadline(t time.Time) error {
+ return s.conn.SetReadDeadline(t)
+}
+
+func (s *ConnSniffer) SetWriteDeadline(t time.Time) error {
+ return s.conn.SetWriteDeadline(t)
+}
diff --git a/src/pkg/net/http/fs_test.go b/src/pkg/net/http/fs_test.go
--- a/src/pkg/net/http/fs_test.go
+++ b/src/pkg/net/http/fs_test.go
@@ -340,11 +340,6 @@
}
func TestServeFileFromCWD(t *testing.T) {
- if runtime.GOOS == "windows" {
- // TODO(brainman): find out why this test is broken
- t.Logf("Temporarily skipping test on Windows; see http://golang.org/issue/3917")
- return
- }
ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
ServeFile(w, r, "fs_test.go")
}))
diff --git a/src/pkg/net/http/transport.go b/src/pkg/net/http/transport.go
--- a/src/pkg/net/http/transport.go
+++ b/src/pkg/net/http/transport.go
@@ -308,7 +308,8 @@
if t.Dial != nil {
return t.Dial(network, addr)
}
- return net.Dial(network, addr)
+ conn, err := net.Dial(network, addr)
+ return NewConnSniffer(conn), err
}
// getConn dials and creates a new persistConn to the target as
Now, I see this:
c:\go\src\pkg\net\http>go test -v -run=FromCWD
warning: building out-of-date packages:
net/http/httptest
net/http/httputil
installing these packages with 'go test -i' will speed future tests.
=== RUN TestServeFileFromCWD
written n=93 data=["GET / HTTP/1.1\r\nHost" ... "t-Encoding: gzip\r\n\r\n"] err=<nil>
read n=188 data=["HTTP/1.1 200 OK\r\nAcc" ... "012 07:09:22 GMT\r\n\r\n"] err=<nil>
...
Blocks here. Connection is still up. What happens to the body of this response? Why http.Get does not read the body? Which part of http package will trigger reading of that?
Alex