Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/net/http2: investigate client write buffering #13593

Open
odeke-em opened this issue Dec 12, 2015 · 2 comments

Comments

@odeke-em
Copy link
Member

commented Dec 12, 2015

testClientWrite in http2 mode with CL https://go-review.googlesource.com/#/c/17751/1 fails, but the fails aren't that consistent

Run 1:

$ go test -cover
--- FAIL: TestClientWrites_h2 (0.00s)
    client_test.go:578: Get request did 8 Write calls, want 1
    client_test.go:587: Post request did 3 Write calls, want 1
FAIL
coverage: 78.6% of statements
exit status 1
FAIL    net/http    23.081s

Run 2:

$ go test -cover
--- FAIL: TestClientWrites_h2 (0.01s)
    client_test.go:578: Get request did 8 Write calls, want 1
FAIL
coverage: 78.5% of statements
exit status 1

The diff that produced this is

diff --git a/src/net/http/client_test.go b/src/net/http/client_test.go
index e72f3bc..05a543c 100644
--- a/src/net/http/client_test.go
+++ b/src/net/http/client_test.go
@@ -550,11 +550,14 @@ func (c *writeCountingConn) Write(p []byte) (int, error) {

 // TestClientWrites verifies that client requests are buffered and we
 // don't send a TCP packet per line of the http request + body.
-func TestClientWrites(t *testing.T) {
+func TestClientWrites_h1(t *testing.T) { testClientWrites(t, h1Mode) }
+func TestClientWrites_h2(t *testing.T) { testClientWrites(t, h2Mode) }
+
+func testClientWrites(t *testing.T, h2 bool) {
    defer afterTest(t)
-   ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
+   cst := newClientServerTest(t, h2, HandlerFunc(func(w ResponseWriter, r *Request) {
    }))
-   defer ts.Close()
+   defer cst.close()

    writes := 0
    dialer := func(netz string, addr string) (net.Conn, error) {
@@ -564,9 +567,10 @@ func TestClientWrites(t *testing.T) {
        }
        return c, err
    }
-   c := &Client{Transport: &Transport{Dial: dialer}}
+   cst.c.Transport.(*Transport).Dial = dialer

-   _, err := c.Get(ts.URL)
+   c := cst.c
+   _, err := c.Get(cst.ts.URL)
    if err != nil {
        t.Fatal(err)
    }
@@ -575,7 +579,7 @@ func TestClientWrites(t *testing.T) {
    }

    writes = 0
-   _, err = c.PostForm(ts.URL, url.Values{"foo": {"bar"}})
+   _, err = c.PostForm(cst.ts.URL, url.Values{"foo": {"bar"}})
    if err != nil {
        t.Fatal(err)
    }
@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 12, 2015

h1 and h2 have different write patterns. This test was locking in a certain behavior for the h1 client.

It's true that the h2 client does more writes than it needs to in some cases (less buffering) for simplicity, but I wasn't planning on investigating whether it's worth fixing until after Go 1.6.

@bradfitz bradfitz changed the title net/http: testClientWrite in http2 mode is broken x/net/http2: investigate client write buffering Dec 12, 2015

@bradfitz bradfitz added this to the Unplanned milestone Dec 12, 2015

@odeke-em

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2015

Aye aye, sounds like a plan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.