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: ResponseRecorder doesn't match reality #15560

Closed
niemeyer opened this issue May 5, 2016 · 10 comments
Closed

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

niemeyer opened this issue May 5, 2016 · 10 comments
Milestone

Comments

@niemeyer
Copy link
Contributor

niemeyer commented May 5, 2016

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 bradfitz changed the title Again httptest.ResponseRecorder doesn't match reality net/http/httptest: ResponseRecorder doesn't match reality May 5, 2016
@bradfitz
Copy link
Contributor

bradfitz commented May 5, 2016

Example?

@bradfitz bradfitz added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label May 5, 2016
@bradfitz bradfitz added this to the Go1.7Maybe milestone May 5, 2016
@niemeyer
Copy link
Contributor Author

niemeyer commented May 5, 2016

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

@niemeyer niemeyer removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label May 5, 2016
@bradfitz
Copy link
Contributor

bradfitz commented May 5, 2016

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
Copy link
Contributor

rsc commented May 17, 2016

@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?

@rsc rsc modified the milestones: Go1.7, Go1.7Maybe May 17, 2016
@bradfitz
Copy link
Contributor

@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
Copy link
Contributor

neild commented May 19, 2016

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
Copy link
Contributor

@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.

@bradfitz bradfitz self-assigned this May 19, 2016
@neild
Copy link
Contributor

neild commented May 19, 2016

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
Copy link
Contributor

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
Copy link

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

@golang golang locked and limited conversation to collaborators May 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants