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/httptest: using Recorder.HeaderMap contains headers that were written after http.ResponseWriter.WriteHeader is called #37650

Closed
elgohr opened this issue Mar 4, 2020 · 17 comments
Milestone

Comments

@elgohr
Copy link

@elgohr elgohr commented Mar 4, 2020

#14914 (comment)
Reproducible in go1.14.

@elgohr

This comment has been minimized.

Copy link
Author

@elgohr elgohr commented Mar 4, 2020

Workaround, use:

res.Result().Header.Get("Content-Type")

Having two ways for accessing the header feels very confusing to me.
Additionally we ask the result for the result...

@dmitshur dmitshur changed the title httptest allows headers to be written after http.ResponseWriter.WriteHeader is called net/http/httptest: allows headers to be written after http.ResponseWriter.WriteHeader is called Mar 4, 2020
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 5, 2020

Change https://golang.org/cl/222117 mentions this issue: net/http/httptest: adds test for #37650

@toothrot toothrot added the NeedsFix label Mar 9, 2020
@toothrot toothrot added this to the Go1.15 milestone Mar 9, 2020
@toothrot

This comment has been minimized.

Copy link
Contributor

@toothrot toothrot commented Mar 9, 2020

/cc @bradfitz

@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Mar 28, 2020

Hello @elgohr, thank you for filing this issue!

Could you please explain what the problem is and provide sample code instead of perhaps a link to a comment in a bug that was fixed?

Did you mean net/http/httptest.ResponseRecorder or net/http/httptest.Server? I can't seem to reproduce this problem and also we have functioning tests that ensure that this doesn't happen.

However, if you declared a Trailer before invoking w.WriteHeader, that trailer will be sent along,
when set later on.

Given that you didn't mention where you are getting the problem but I can allude to the usage of net/http/httptest.ResponseRecorder, I shall provide you with 2 different scenarios, and please tweak the code to see if it reproduces your issue.

1. net/http/httptest.ResponseRecorder

package main

import (
        "net/http"
        "net/http/httptest"
        "net/http/httputil"
        "testing"
)

func handler(w http.ResponseWriter, r *http.Request) {
        // Declare this Trailer that will be permissible after
        // after w.WriteHeader is invoked.
        w.Header().Set("Trailer", "AB")
        w.WriteHeader(200)
        w.Write([]byte("Aloha"))
        // Throw in this stray header that was not declared as a Trailer.
        w.Header().Set("Stray", "Bar")
        w.Header().Set("AB", "CD")
}

func TestHeaderWriteInResponseRecorder(t *testing.T) {
        req := httptest.NewRequest("GET", "https://example.com/", nil)
        rec := httptest.NewRecorder()

        handler(rec, req)
        resp := rec.Result()
        defer resp.Body.Close()

        blob, _ := httputil.DumpResponse(resp, true)
        want := "HTTP/1.1 200 OK\r\nConnection: close\r\n\r\nAloha"

        if g, w := string(blob), want; g != w {
                t.Fatalf("Response mismatch\nGot:\n%q\n\nWant:\n%q", g, w)
        }
        if g, w := resp.Trailer.Get("AB"), "CD"; g != w {
                t.Fatalf("Trailer mismatch\nGot:  %q\nWant: %q", g, w)
        }
        if g := resp.Header["Stray"]; g != nil {
                t.Fatalf("Stray header unfortunately sent after w.WriteHeader(...), got: %#v", g)
        }
}
$ go test -v
=== RUN   TestHeaderWriteInResponseRecorder
--- PASS: TestHeaderWriteInResponseRecorder (0.00s)
PASS
ok  	github.com/odeke-em/bugs/golang/37650	0.030s

2. net/http/httptest.Server

Could you perhaps please try tweaking this code to see if it reproduces your issue?

package main

import (
	"net/http"
	"net/http/httptest"
	"net/http/httputil"
	"testing"
)

func handler(w http.ResponseWriter, r *http.Request) {
	// Declare this Trailer that will be permissible after
	// after w.WriteHeader is invoked.
	w.Header().Set("Trailer", "AB")
	w.WriteHeader(200)
	w.Write([]byte("Aloha"))
	// Throw in this stray header that was not declared as a Trailer.
	w.Header().Set("Stray", "Bar")
	w.Header().Set("AB", "CD")
}

func TestHeaderWrite_h1(t *testing.T) {
	testHeaderWrite(t, false)
}
func TestHeaderWrite_h2(t *testing.T) {
	testHeaderWrite(t, true)
}

func testHeaderWrite(t *testing.T, isHTTP2 bool) {
	cst := httptest.NewUnstartedServer(http.HandlerFunc(handler))
	cst.EnableHTTP2 = isHTTP2
	cst.StartTLS()
	defer cst.Close()

	req, _ := http.NewRequest("GET", cst.URL, nil)
	resp, err := cst.Client().Do(req)
	if err != nil {
		panic(err)
	}
	defer resp.Body.Close()
	// firstly strip out ephemeral header: "Date"
	delete(resp.Header, "Date")

	blob, _ := httputil.DumpResponse(resp, true)
	want := "HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\nTrailer: Ab\r\nContent-Type: text/plain; charset=utf-8\r\n\r\n5\r\nAloha\r\n0\r\nAb: CD\r\n\r\n"
	if isHTTP2 {
		want = "HTTP/2.0 200 OK\r\nContent-Length: 5\r\nContent-Type: text/plain; charset=utf-8\r\n\r\nAloha"
	}

	if g, w := string(blob), want; g != w {
		t.Fatalf("Response mismatch\nGot:\n%q\n\nWant:\n%q", g, w)
	}
	if g, w := resp.Trailer.Get("AB"), "CD"; g != w {
		t.Fatalf("Trailer mismatch\nGot:  %q\nWant: %q", g, w)
	}
	if g := resp.Header["Stray"]; g != nil {
		t.Fatalf("Stray header unfortunately sent after w.WriteHeader(...), got: %#v", g)
	}
}

and currently it passes with

$ go test -v
=== RUN   TestHeaderWrite_h1
--- PASS: TestHeaderWrite_h1 (0.00s)
=== RUN   TestHeaderWrite_h2
--- PASS: TestHeaderWrite_h2 (0.00s)
PASS
ok  	github.com/odeke-em/bugs/golang/37650	0.019s

Workaround, use:

res.Result().Header.Get("Content-Type")
Having two ways for accessing the header feels very confusing to me.
Additionally we ask the result for the result..

I don't follow what you mean, please elaborate.

Thank you!

@odeke-em odeke-em added WaitingForInfo and removed NeedsFix labels Mar 28, 2020
@elgohr

This comment has been minimized.

Copy link
Author

@elgohr elgohr commented Mar 29, 2020

Hey @odeke-em did you see the test in the PR? In this way you could reproduce and see the issue.

@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Mar 29, 2020

@elgohr please use recorder.Result() to compare apples to apples otherwise you are using an attribute HeaderMap that was deprecated since Go1.11, and is meant to be an internal attribute
Screen Shot 2020-03-28 at 11 09 43 PM

As I demonstrated in the tests in #37650 (comment), if you use ResponseRecorder().Result() to retrieve the Response and then compare against that, that'll give you the correct results.

@odeke-em odeke-em changed the title net/http/httptest: allows headers to be written after http.ResponseWriter.WriteHeader is called net/http/httptest: using Recorder.HeaderMap contains headers that were written after http.ResponseWriter.WriteHeader is called Mar 29, 2020
@elgohr

This comment has been minimized.

Copy link
Author

@elgohr elgohr commented Mar 29, 2020

So why is recorder.Header() still exported and non-deprecated?

@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Mar 29, 2020

@elgohr

This comment has been minimized.

Copy link
Author

@elgohr elgohr commented Mar 29, 2020

So this should probably be the default method for checking the "right" headers? Otherwise it's very confusing, as it's even returning the "wrong" headers.

@elgohr

This comment has been minimized.

Copy link
Author

@elgohr elgohr commented Mar 29, 2020

I wouldn't go with #38149 .
Summing up this issue: we have two methods for getting the response header in the recorder. Both return headers. One is needed for http.ResponseWriter. The needed one returns deprecated headers, the non-needed returns the correct headers. This doesn't look intuitive to me.

@elgohr elgohr closed this Mar 29, 2020
@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Mar 29, 2020

@elgohr elgohr reopened this Mar 29, 2020
@elgohr

This comment has been minimized.

Copy link
Author

@elgohr elgohr commented Mar 29, 2020

Sorry, big thumb on mobile app closed this issue. So reopen

@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Mar 29, 2020

@elgohr

This comment has been minimized.

Copy link
Author

@elgohr elgohr commented Mar 29, 2020

I see your point. I'm just saying that the current behavior is very unintuitive to me. A method name is like a promise. If I would have to look up the documentation for every promise, we could also call the method x, y or z (see https://www.lysator.liu.se/c/pikestyle.html - "function names should reflect what they return")

@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Mar 29, 2020

@elgohr

This comment has been minimized.

Copy link
Author

@elgohr elgohr commented Mar 29, 2020

So why can't .Headers() return the non-deprecated Headers?

In 2f33e25#diff-ee0abc83acc8d7e468b0773ae2dd9ea5R288 I implemented the test the other way around by accident. It reflects that fields, which will never be returned by the server (as set too late) are shown as set. This is simply wrong.

@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Mar 29, 2020

So why can't .Headers() return the non-deprecated Headers?

.Headers() is a method that's intended for use inside the server aka http.Handler surface, and not in the client API surface. There isn't a way that you can use an item to satisfy an interface i.e. http.ResponseWriter without implementing its methods.

In 2f33e25#diff-ee0abc83acc8d7e468b0773ae2dd9ea5R288 I implemented the test the other way around by accident. It reflects that fields, which will never be returned by the server (as set too late) are shown as set. This is simply wrong.

That is not using the right APIs, perhaps this list can be helpful:
a) to operate on Responses, please use .Result()
b) to use the server-side APIs aka in handler, use the methods for http.ResponseWriter

Using methods meant for the http.ResponseWriter is out of scope for when you expect to operate on an *http.Response.

In the future, if you'd like to ask questions, please check out these resources at https://github.com/golang/go/wiki/Questions, or inlined below:

I am going to close this issue as it is working as intended and conflation of APIs.
Thank you for filing it though and for the conversation.

@odeke-em odeke-em closed this Mar 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.