Proposal #54136 (implemented in CL 436890 which is part of Go 1.20) added the "http".ResponseController type, which allows manipulating per-request timeouts. This is especially useful for programs managing long-running HTTP connections such as Mercure.
However, testing HTTP handlers leveraging per-request timeouts is currently cumbersome (even if doable) because "net/http/httptest".ResponseRecorder isn't compatible yet with "http".ResponseController.
To make it fully compatible with response controllers and improve the testing experience, I propose the following additions to its public API:
type ResponseRecorder struct {
// ...
// ReadDeadline is the last read deadline that has been set using
// "net/http".ResponseController
ReadDeadline time.Time
// WriteDeadline is the last write deadline that has been set using
// "net/http".ResponseController
WriteDeadline time.Time
}
// SetReadDeadline allows using "net/http".ResponseController.SetReadDeadline()
// with the recorder.
//
// The deadline is recorded but is not enforced.
// To prevent flaky tests reads made after the deadline will work
// as if no deadline was set.
//
// To retrieve the deadline, use rw.ReadDeadline.
func (rw *ResponseRecorder) SetReadDeadline(deadline time.Time) error
// SetWriteDeadline allows using "net/http".ResponseController.SetWriteDeadline()
// with the recorder.
//
// The deadline is recorded but is not enforced.
// To prevent flaky tests writes made after the deadline will work
// as if no deadline was set.
//
// To retrieve the deadline, use rw.WriteDeadline.
func (rw *ResponseRecorder) SetWriteDeadline(deadline time.Time) error
All new methods are part of the contract that response types must honor to be usable with "HTTP".ResponseController.
As discussed below, deadlines are recorded but not enforced to prevent flaky tests.
Proposal implementation: #60231
Comment From: gopherbot
Change https://go.dev/cl/495295 mentions this issue: net/http/httptest: add support for http.ResponseController to ResponseRecorder
Comment From: neild
I don't think ResponseRecorder should be trying to apply a read or write deadline.
It can't apply a real read deadline, since it has no good way to interrupt reads from the request body. In CL 495295, setting a read deadline causes req.Body.Read calls made after the deadline has passed to return an error, but doesn't do anything to interrupt in-progress calls.
Setting a write deadline can make write calls after the deadline has passed fail. Since ResponseRecorder writes to a bytes.Buffer, interrupting in-progress writes is not a concern.
Making reads and writes fail in this fashion seems out of scope for ResponseRecorder (which just records responses).
This also seems like an encouragement to write flaky tests. For example, in the example:
rc := http.NewResponseController(w)
rc.SetReadDeadline(time.Now().Add(1 * time.Second))
rc.SetWriteDeadline(time.Now().Add(3 * time.Second))
io.WriteString(w, "<html><body>Hello, with deadlines!</body></html>")
This may or may not write the response string to the recorder. If three seconds pass between the call to SetWriteDeadline and the WriteString call, nothing is written. This might seem unlikely, but much test flakiness stems from exactly this sort of case when a slow test machine pauses for an unexpectedly long time between operations.
If you do want to write tests that rely on a record ResponseController which supports deadlines, it's simple to compose ResponseRecorder with a custom controller that implements SetReadDeadline and SetWriteDeadline:
type timeoutResponseWriter struct {
httptest.ResponseRecorder
}
func (w *timeoutResponseWriter) Unwrap() http.ResponseWriter {
return &w.ResponseRecorder // for Flush
}
func (w *timeoutResponseWriter) SetReadDeadline(deadline time.Duration) error {
// ...
}
Comment From: dunglas
Thanks for the quick and detailed feedback @neild.
My main goal was to be able to easily assert that a deadline has been set (or changed) by the handler under test. I thought it was nice to have the full contract needed by "http".ResponseController implemented and to return an error "as in prod" if reached, but indeed this can encourage writing flaky tests.
It's definitely doable to implement a custom type similar to what is in this PR (it's already what we do for Mercure), but the developer experience is usually better when it's not necessary to write custom code to test native features.
Do you think that there is value in providing the methods to set deadlines but not enforce them (to prevent flaky tests), or should I close this proposal and the related CL?
Comment From: neild
Recording deadlines and not enforcing them would fit within ResponseRecorder's remit. I worry a bit that it'd lead to confusion from users who expect the deadline to be enforced, but ResponseRecorder doesn't really lend itself to use outside of tests so that's probably not a significant concern.
So I'd be fine with adding record-only deadline methods.
Comment From: dunglas
I updated the proposal and the related patch accordingly.
Comment From: dunglas
As deadlines can be updated during the HTTP handler execution, would it be interesting to record all calls to Set* methods?
The API would then be:
type ResponseRecorder struct {
// ...
// ReadDeadline is the list of read deadlines that have been set using
// "net/http".ResponseController
ReadDeadlines []time.Time
// WriteDeadline is the list of write deadlines that have been set using
// "net/http".ResponseController
WriteDeadlines []time.Time
}
// SetReadDeadline allows using "net/http".ResponseController.SetReadDeadline()
// with the recorder.
//
// The deadline is recorded but is not enforced.
// To prevent flaky tests reads made after the deadline will work
// as if no deadline was set.
//
// To retrieve the deadlines, use rw.ReadDeadlines.
func (rw *ResponseRecorder) SetReadDeadline(deadline time.Time) error
// SetWriteDeadline allows using "net/http".ResponseController.SetWriteDeadline()
// with the recorder.
//
// The deadline is recorded but is not enforced.
// To prevent flaky tests writes made after the deadline will work
// as if no deadline was set.
//
// To retrieve the deadlines, use rw.WriteDeadlines.
func (rw *ResponseRecorder) SetWriteDeadline(deadline time.Time) error
Comment From: neild
(Sorry for losing track of this.)
As deadlines can be updated during the HTTP handler execution, would it be interesting to record all calls to Set* methods?
I think that this suggestion has moved me in the direction of thinking that we should not add any support for Set*Deadline to ResponseRecorder.
Recording deadlines has the fundamental problem that we don't know when the deadline was set. For example, whether we record a single SetReadDeadline call or all of them, a test cannot distinguish between the correct handler:
func(w http.ResponseWriter, req *http.Request) {
http.NewResponseController(w).SetReadDeadline(time.Now().Add(1 * time.Second)
io.Copy(w, respBody)
)
And the (probably) incorrect handler:
func(w http.ResponseWriter, req *http.Request) {
io.Copy(w, respBody)
http.NewResponseController(w).SetReadDeadline(time.Now().Add(1 * time.Second)
)
This gets worse if we have multiple deadlines applied across the course of the handler. I don't see how you can write a good test that asserts something like "the handler set deadlines of 1s, 10s, and 10s" without reference to what portions of the response those deadlines applied to.
As I mentioned in https://github.com/golang/go/issues/60229#issuecomment-1550053778, if you really do want to do that style of recording, it isn't difficult to compose ResponseRecorder with a custom ResponseWriter.
Comment From: rsc
Based on the most recent reply, it sounds like we should not do this?
Comment From: rsc
This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group
Comment From: rsc
Based on the discussion above, this proposal seems like a likely decline. — rsc for the proposal review group
Comment From: rsc
No change in consensus, so declined. — rsc for the proposal review group