net/http/httptest: ResponseRecorder doesn't match reality

[niemeyer]

In an attempt to fix the behavior of httptest.ResponseRecorder to mimic reality more closely (issues #8857 and #14914), commit c69e686 broke reality in the opposite end: now the recorder will not display headers unless one of Write, WriteHeader, or Flush, is called explicitly, which is not what happens on an actual handler.

This broke actual tests we had in 1.6 after running with the httptest package from tip.

As a suggested fix for both cases, the recorded headers should be frozen when Write or WriteHeader is observed, rather than completely hidden from tests until then.

[bradfitz]

Example?

[niemeyer]

Please try this on tip: https://play.golang.org/p/b2MpIHfdCn

[bradfitz]

We can't have both. The original API was kinda bogus in that it lacked a place to run code after ServeHTTP completes (like the actual http server does). We might need a helper function or method in the httptest package to run an http.Handler for you.

[rsc]

@bradfitz, I don't understand "we can't have both". What's wrong with making the headers work for Get until the first Write or WriteHeader and then freezing a copy at that point?

[bradfitz]

@rsc, that's what @cmarcelo's recent CL did, which broke the use case @niemeyer is reporting.

The fundamental problem is that ResponseWriter.HeaderMap is one map but people want it to mean two things:

1. the return value from ResponseWriter.Header(). There is no freezing this map at Write or Flush, since it's just a Go map. Once the user has it in their hands, all bets are off.
2. the effective written headers (the "frozen" set), ignoring any mutations the user made after the first Write or Flush.

Historically, we didn't do any freezing, which is why people reported bugs like #8857. In an attempt to fix that, @cmarcelo made ResponseWriter.Header() return a private map which later snapshotted into the public field. However, since it's the user running their ServerHTTP handler and not us, we didn't know when the function was over, so there was no final snapshot in the case where the handler never did a flush. (which both the HTTP/1.x and HTTP/2 servers do)

I think the only safe option here is to revert the behavior back to Go1..Go1.6 behavior where HeaderMap is partially bogus (but doesn't break people's old tests), and create a new method to return the wire header map for people who want the fixed behavior. That new method could say that it can't be called until after the handler is done running, and that's where we can do the final snapshot if necessary.

[neild]

How about:

1. ResponseWriter.Header() returns ResponseWriter.HeaderMap if the headers are unfrozen.
2. ResponseWriter.Header() returns a copy of ResponseWriter.HeaderMap if the headers are frozen.
3. Write and Flush freeze the headers and replace HeaderMap with a copy of itself.

[bradfitz]

@neild, that nearly works, but not quite. Replacing HeaderMap doesn't work, because once it's set, people can get at it.

I have a change in progress. I'll send it to you.

[neild]

People can get at the HeaderMap in general, but HTTP handlers under test can't. At least, not without type asserting to ResponseWriter, which is obviously broken.

[bradfitz]

See https://go-review.googlesource.com/23257

People create ResponseRecorders both by hand and with the constructor. The API is a bit regrettable at this point, but I don't want to tell people how to use it. It's too old at this point.

My CL keeps the old behavior so we don't break people's tests, and simply adds a new func (like the existing new func already added in Go 1.6 for trailers) for people who want the fixed behavior.

[gopherbot]

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