proposal: net/http/httptest: add support for http.ResponseController to ResponseRecorder #60229

dunglas · 2023-05-16T16:06:59Z

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

The text was updated successfully, but these errors were encountered:

ResponseRecorder 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. This patch makes ResponseRecorder compatible with "http".ResponseController. All new methods are part of the contract that response types must honor to be usable with "http".ResponseController. NewRecorderWithDeadlineAwareRequest() is necessary to test read deadlines, as calling rw.SetReadDeadline() must change the deadline on the request body. Fixes golang#60229.

ResponseRecorder golang#54136 (implemented in 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. This patch makes ResponseRecorder compatible with "http".ResponseController. All new methods are part of the contract that response types must honor to be usable with "http".ResponseController. NewRecorderWithDeadlineAwareRequest() is necessary to test read deadlines, as calling rw.SetReadDeadline() must change the deadline on the request body. Fixes golang#60229. # Veuillez saisir le message de validation pour vos modifications. Les lignes # commençant par '#' seront conservées ; vous pouvez les supprimer vous-même # si vous le souhaitez. Un message vide abandonne la validation. # # Date : Mon May 15 17:59:56 2023 +0200 # # Sur la branche httptest_recorder_responsecontroller # Modifications qui seront validées : # modifié : net/http/httptest/example_test.go # modifié : net/http/httptest/recorder.go # modifié : net/http/httptest/recorder_test.go # modifié : net/http/server.go #

ResponseRecorder CL golang#54136 (implemented in 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. This patch makes ResponseRecorder compatible with "http".ResponseController. All new methods are part of the contract that response types must honor to be usable with "http".ResponseController. NewRecorderWithDeadlineAwareRequest() is necessary to test read deadlines, as calling rw.SetReadDeadline() must change the deadline on the request body. Fixes golang#60229.

…rder CL golang#54136 (implemented in 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. This patch makes ResponseRecorder compatible with "http".ResponseController. All new methods are part of the contract that response types must honor to be usable with "http".ResponseController. NewRecorderWithDeadlineAwareRequest() is necessary to test read deadlines, as calling rw.SetReadDeadline() must change the deadline on the request body. Fixes golang#60229.

gopherbot · 2023-05-16T16:31:31Z

Change https://go.dev/cl/495295 mentions this issue: net/http/httptest: add support for http.ResponseController to ResponseRecorder

neild · 2023-05-16T17:06:44Z

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 {
  // ...
}

dunglas · 2023-05-16T21:03:17Z

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?

neild · 2023-05-16T21:13:01Z

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.

dunglas · 2023-05-17T10:04:23Z

I updated the proposal and the related patch accordingly.

dunglas · 2023-05-18T07:43:07Z

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

once the fix for golang/go#60229 makes it to the go version, this commit can be dropped

neild · 2024-03-22T22:15:19Z

(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 #60229 (comment), if you really do want to do that style of recording, it isn't difficult to compose ResponseRecorder with a custom ResponseWriter.

once the fix for golang/go#60229 makes it to the go version, this commit can be dropped

rsc · 2024-04-24T17:35:25Z

Based on the most recent reply, it sounds like we should not do this?

rsc · 2024-04-24T19:09:58Z

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

rsc · 2024-05-08T21:19:42Z

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

rsc · 2024-05-15T18:18:28Z

No change in consensus, so declined.
— rsc for the proposal review group

dunglas added the Proposal label May 16, 2023

gopherbot added this to the Proposal milestone May 16, 2023

dunglas mentioned this issue May 16, 2023

net/http/httptest: add support for http.ResponseController to ResponseRecorder #60231

Open

ianlancetaylor added this to Proposals May 16, 2023

ianlancetaylor moved this to Incoming in Proposals May 16, 2023

komuw mentioned this issue Sep 1, 2023

track go/issues/60229: net/http/httptest: add support for http.ResponseController to ResponseRecorder komuw/ong#377

Open

tkashem added a commit to tkashem/kubernetes that referenced this issue Jan 15, 2024

add support for http.ResponseController to ResponseRecorder

c97ef8a

once the fix for golang/go#60229 makes it to the go version, this commit can be dropped

tkashem added a commit to tkashem/kubernetes that referenced this issue Jan 15, 2024

add support for http.ResponseController to ResponseRecorder

a2eed42

once the fix for golang/go#60229 makes it to the go version, this commit can be dropped

tkashem added a commit to tkashem/kubernetes that referenced this issue Jan 18, 2024

add support for http.ResponseController to ResponseRecorder

38be2a1

once the fix for golang/go#60229 makes it to the go version, this commit can be dropped

tkashem added a commit to tkashem/kubernetes that referenced this issue Jan 19, 2024

add support for http.ResponseController to ResponseRecorder

87e9817

once the fix for golang/go#60229 makes it to the go version, this commit can be dropped

tkashem added a commit to tkashem/kubernetes that referenced this issue Jan 19, 2024

add support for http.ResponseController to ResponseRecorder

39402ec

once the fix for golang/go#60229 makes it to the go version, this commit can be dropped

tkashem added a commit to tkashem/kubernetes that referenced this issue Jan 19, 2024

add support for http.ResponseController to ResponseRecorder

2261e08

once the fix for golang/go#60229 makes it to the go version, this commit can be dropped

tkashem added a commit to tkashem/kubernetes that referenced this issue Jan 20, 2024

add support for http.ResponseController to ResponseRecorder

45d9523

once the fix for golang/go#60229 makes it to the go version, this commit can be dropped

tkashem added a commit to tkashem/kubernetes that referenced this issue Jan 20, 2024

add support for http.ResponseController to ResponseRecorder

0119dea

once the fix for golang/go#60229 makes it to the go version, this commit can be dropped

tkashem added a commit to tkashem/kubernetes that referenced this issue Jan 20, 2024

add support for http.ResponseController to ResponseRecorder

2324a6e

once the fix for golang/go#60229 makes it to the go version, this commit can be dropped

tkashem added a commit to tkashem/kubernetes that referenced this issue Jan 22, 2024

add support for http.ResponseController to ResponseRecorder

9b78a5e

once the fix for golang/go#60229 makes it to the go version, this commit can be dropped

tkashem added a commit to tkashem/kubernetes that referenced this issue Jan 22, 2024

add support for http.ResponseController to ResponseRecorder

4bf80c0

once the fix for golang/go#60229 makes it to the go version, this commit can be dropped

tkashem added a commit to tkashem/kubernetes that referenced this issue Jan 22, 2024

add support for http.ResponseController to ResponseRecorder

7ef54ad

once the fix for golang/go#60229 makes it to the go version, this commit can be dropped

tkashem added a commit to tkashem/kubernetes that referenced this issue Jan 22, 2024

add support for http.ResponseController to ResponseRecorder

d58b319

once the fix for golang/go#60229 makes it to the go version, this commit can be dropped

tkashem added a commit to tkashem/kubernetes that referenced this issue Jan 22, 2024

add support for http.ResponseController to ResponseRecorder

f70a463

once the fix for golang/go#60229 makes it to the go version, this commit can be dropped

tkashem added a commit to tkashem/kubernetes that referenced this issue Apr 15, 2024

add support for http.ResponseController to ResponseRecorder

e787463

once the fix for golang/go#60229 makes it to the go version, this commit can be dropped

rsc moved this from Incoming to Active in Proposals Apr 24, 2024

rsc moved this from Active to Likely Decline in Proposals May 8, 2024

rsc added the Proposal-FinalCommentPeriod label May 8, 2024

rsc moved this from Likely Decline to Declined in Proposals May 15, 2024

rsc closed this as completed May 15, 2024

rsc removed the Proposal-FinalCommentPeriod label May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proposal: net/http/httptest: add support for http.ResponseController to ResponseRecorder #60229

proposal: net/http/httptest: add support for http.ResponseController to ResponseRecorder #60229

dunglas commented May 16, 2023 •

edited by gabyhelp

Loading

gopherbot commented May 16, 2023

neild commented May 16, 2023

dunglas commented May 16, 2023

neild commented May 16, 2023

dunglas commented May 17, 2023

dunglas commented May 18, 2023

neild commented Mar 22, 2024

rsc commented Apr 24, 2024

rsc commented Apr 24, 2024

rsc commented May 8, 2024

rsc commented May 15, 2024

proposal: net/http/httptest: add support for http.ResponseController to ResponseRecorder #60229

proposal: net/http/httptest: add support for http.ResponseController to ResponseRecorder #60229

Comments

dunglas commented May 16, 2023 • edited by gabyhelp Loading

gopherbot commented May 16, 2023

neild commented May 16, 2023

dunglas commented May 16, 2023

neild commented May 16, 2023

dunglas commented May 17, 2023

dunglas commented May 18, 2023

neild commented Mar 22, 2024

rsc commented Apr 24, 2024

rsc commented Apr 24, 2024

rsc commented May 8, 2024

rsc commented May 15, 2024

dunglas commented May 16, 2023 •

edited by gabyhelp

Loading