Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(313)

Issue 6553061: code review 6553061: net/http: use r.Body.Close to close connection during T... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 6 months ago by brainman
Modified:
11 years, 6 months ago
Reviewers:
CC:
golang-dev, bradfitz
Visibility:
Public.

Description

net/http: use r.Body.Close to close connection during TestServeFileFromCWD Fixes issue 3917.

Patch Set 1 #

Patch Set 2 : diff -r 6fdbfbfa8813 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 6fdbfbfa8813 https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r 6fdbfbfa8813 https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -5 lines) Patch
M src/pkg/net/http/fs_test.go View 1 2 chunks +1 line, -5 lines 0 comments Download

Messages

Total messages: 6
brainman
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
11 years, 6 months ago (2012-09-24 02:29:35 UTC) #1
bradfitz
LGTM but leaking 1 fd was enough to make the windows builders flaky? On Sun, ...
11 years, 6 months ago (2012-09-24 02:31:36 UTC) #2
brainman
On 2012/09/24 02:31:36, bradfitz wrote: > > but leaking 1 fd was enough to make ...
11 years, 6 months ago (2012-09-24 02:46:10 UTC) #3
brainman
*** 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 ...
11 years, 6 months ago (2012-09-24 02:48:27 UTC) #4
bradfitz
On Sun, Sep 23, 2012 at 7:46 PM, <alex.brainman@gmail.com> wrote: > On 2012/09/24 02:31:36, bradfitz ...
11 years, 6 months ago (2012-09-24 05:37:21 UTC) #5
brainman
11 years, 6 months ago (2012-09-24 07:33:59 UTC) #6
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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b