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

net/http: discrepancy between HeadContentLength of "" in http2 vs http1 #13532

Closed
odeke-em opened this issue Dec 8, 2015 · 3 comments

Comments

Projects
None yet
3 participants
@odeke-em
Copy link
Member

commented Dec 8, 2015

HeadContentLength of "" in http1 returns a length of -1 but in http2 it returns 0.
The test that found this issue is through CL https://go-review.googlesource.com/17526

diff --git a/src/net/http/client_test.go b/src/net/http/client_test.go
index e59ab2c..6e20877 100644
--- a/src/net/http/client_test.go
+++ b/src/net/http/client_test.go
@@ -765,14 +765,22 @@ func TestHTTPSClientDetectsHTTPServer(t *testing.T) {
 }

 // Verify Response.ContentLength is populated. https://golang.org/issue/4126
-func TestClientHeadContentLength(t *testing.T) {
+func TestClientHeadContentLength_h1(t *testing.T) {
+   testClientHeadContentLength(t, false)
+}
+
+func TestClientHeadContentLength_h2(t *testing.T) {
+   testClientHeadContentLength(t, true)
+}
+
+func testClientHeadContentLength(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) {
        if v := r.FormValue("cl"); v != "" {
            w.Header().Set("Content-Length", v)
        }
    }))
-   defer ts.Close()
+   defer cst.close()
    tests := []struct {
        suffix string
        want   int64
@@ -782,8 +790,8 @@ func TestClientHeadContentLength(t *testing.T) {
        {"", -1},
    }
    for _, tt := range tests {
-       req, _ := NewRequest("HEAD", ts.URL+tt.suffix, nil)
-       res, err := DefaultClient.Do(req)
+       req, _ := NewRequest("HEAD", cst.ts.URL+tt.suffix, nil)
+       res, err := cst.c.Do(req)
        if err != nil {
            t.Fatal(err)
        }

where the test cases

        tests := []struct {
                suffix string
                want   int64
        }{
                {"/?cl=1234", 1234},
                {"/?cl=0", 0},
                {"", -1},
        }
        for _, tt := range tests {
                req, _ := NewRequest("HEAD", cst.ts.URL+tt.suffix, nil)
                res, err := cst.c.Do(req)
                if err != nil {
                        t.Fatal(err)
                }
                if res.ContentLength != tt.want {
                        t.Errorf("Content-Length = %d; want %d", res.ContentLength, tt.want)
                }
                bs, err := ioutil.ReadAll(res.Body)
                if err != nil {
                        t.Fatal(err)
                }
                if len(bs) != 0 {
                        t.Errorf("Unexpected content: %q", bs)
                }
        }

Gives result

$ go test -cover
--- FAIL: TestClientHeadContentLength_h2 (0.01s)
    client_test.go:799: Content-Length = 0; want -1

@bradfitz bradfitz added this to the Go1.6 milestone Dec 8, 2015

@bradfitz bradfitz self-assigned this Dec 8, 2015

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 8, 2015

Thanks!

@gopherbot

This comment has been minimized.

Copy link

commented Dec 8, 2015

CL https://golang.org/cl/17590 mentions this issue.

@gopherbot

This comment has been minimized.

Copy link

commented Dec 9, 2015

CL https://golang.org/cl/17591 mentions this issue.

bradfitz added a commit to golang/net that referenced this issue Dec 9, 2015

http2: fix two cases of Server behavior not matching HTTP/1
* don't send automatic Content-Length from Server on HEAD requests if
  response size is 0 (it's possible the handler didn't write because
  they looked at the method)

* don't send Content-Type if handler explicitly set it to nothing.

Matches the behavior of HTTP/1.

Updates golang/go#13532
Updates golang/go#13495

Change-Id: If6e0898095cf88cb14efb6bbe82c88dbc2077e6b
Reviewed-on: https://go-review.googlesource.com/17590
Reviewed-by: Blake Mizerany <blake.mizerany@gmail.com>

@bradfitz bradfitz closed this in 3d3d6eb Dec 9, 2015

@golang golang locked and limited conversation to collaborators Dec 14, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.