net/http: TimeoutHandler prevents use of ResponseController

[Acconut]

Go version

go version go1.23.1 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/marius/Library/Caches/go-build'
GOENV='/Users/marius/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/marius/workspace/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/marius/workspace/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/opt/homebrew/Cellar/go/1.23.1/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='local'
GOTOOLDIR='/opt/homebrew/Cellar/go/1.23.1/libexec/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.23.1'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/marius/Library/Application Support/go/telemetry'
GCCGO='gccgo'
GOARM64='v8.0'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/9c/rl48j6r51g37vvhq4vdywz280000gn/T/go-build1640827140=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

I have a http.Handler that uses http.NewResponseController on the response writer to control the response behavior. If my handler is wrapped in a http.TimeoutHandler, calls to ResponseController.SetReadTimeout, ResponseController.EnableFullDuplex return an error saying feature not supported.

https://go.dev/play/p/Bar7glioldQ contains an example for reproduction. Unfortunately, running the code in the playground often fails with an i/o timeout, but locally this problem does not appear.

What did you see happen?

Any interaction with the response controller functions returns an error saying feature not supported. Running the linked code locally produces the following output:

2024/10/04 10:35:12 feature not supported
Hello, client

The reason seems to be that TimeoutHandler wraps the response writer into its own timeoutWriter (https://cs.opensource.google/go/go/+/refs/tags/go1.23.2:src/net/http/server.go;l=3658), which neither implements the methods for ResponseController (https://pkg.go.dev/net/http#NewResponseController) nor an Unwrap function returning the original response writer. The easiest solution might be to add an Unwrap method to timeoutWriter.

I also checked if other handler wrappers in net/http interfere with ResponseController. AllowQuerySemicolons, StripPrefix, and MaxBytesHandler don't cause issues as they don't wrap the response writer.

What did you expect to see?

ResponseController should be usable together with TimeoutHandler. Running the linked code should produce the following output:

Hello, client

[gabyhelp]

Related Issues and Documentation

• net/http: ResponseController limitations #63656 (closed)
• TimeoutHandler and http.Server timeout usage issue at same time #35316 (closed)
• net/http: HTTP client hangs indefinitely if the request handler does not return #65526
• net/http: ResponseController may cause infinite loop #66350 (closed)
• net/http: problems or undocumented features dealing with handler timeout #6410 (closed)
• net/http: ResponseWriter.Write does not error after WriteTimeout nor is ErrorLog used #21389
• net/http: ResponseController to manipulate per-request timeouts (and other behaviors) #54136 (closed)
• net/http: TimeoutHandler is incompatible with a ReverseProxy that has a negative FlushInterval #34198 (closed)
• net/http: Request context is not canceled when `Server.WriteTimeout` is reached #59602
• net/http: server does not send response on timeout #47229

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[mknyszek]

CC @neild

[seankhliao]

One of ResponseController's features is for adjusting per request tiemouts, but it can't change the timeouts on a timeout handler. Seems it would be unintuitive for responsecontroller to succeed but then have any timeouts still be limited by the original timeout handler.

[neild]

The original ResponseController proposal (#54136) did explicitly mention TimeoutHandler, so we didn't forget about it at the time:

The Handler returned by http.TimeoutHandler currently receives a ResponseWriter which does not implement the Flush or Hijack methods. This will not change under this proposal: The *ResponseController for a TimeoutHandler will return a not-implemented error from Flush, Hijack, SetWriteDeadline, and SetReadDeadline.

I don't remember why we specified it that way; it's possible we just felt that figuring out how TimeoutHandler and ResponseController timeouts interacted was a distraction from the proposal.

I'm not sure what the correct behavior here is. How should the ResponseController deadlines interact with the TimeoutHandler deadline? Are they entirely separate? Should you be able to change the TimeoutHandler deadline using SetWriteDeadline?

[Acconut]

Thank you for the additional context, that's helpful!

There is indeed an logical overlap between the timeout from TimeoutHandler and the read/write deadlines from ResponseController. If the deadline from TimeoutHandler is reached, it makes sense to cancel all active reads from the request by resetting the read deadline. In addition, writes to the ResponseWriter that is wrapped by TimeoutHandler should also be cancelled, as they won't reach the client anymore. The write deadline for the underlying ResponseWriter should be extended by a short period to allow TimeoutHandler to write its error response.

When setting the read/write deadline via ResponseWriter, I expect the timeout for TimeoutHandler to not change. This allows the handler to react to reached read/write deadlines and respond to the client properly.

What do you think about this?

[neild]

To recap:

• TimeoutHandler's timeout is fixed. You can't change it.
• Within a TimeoutHandler, you can change the read/write deadline. This just affects reading from the request body and writing to the response body. You can extend the deadlines past the end of the TimeoutHandler if you want, but this probably isn't useful.
• When TimeoutHandler's timeout expires, it sets the read/write deadlines to the current time.

Is that right? If so, seems reasonable to me.

Perhaps TimeoutHandler should ensure that SetReadDeadline/SetWriteDeadline don't extend the deadlines after the TimeoutHandler deadline has passed.

While we're in here, we should probably ensure that ResponseController.EnableFullDuplex works in a TimeoutHandler. Hijack should still return an unimplemented error. Flush should either return an unimplemented error or work correctly.

[Acconut]

Is that right? If so, seems reasonable to me.

I agree.

Perhaps TimeoutHandler should ensure that SetReadDeadline/SetWriteDeadline don't extend the deadlines after the TimeoutHandler deadline has passed.

Depending on how this would be implemented, it could be the source of a race condition. If the TimeoutHandler and the read/write deadlines are set to the same point in time and this point is reached, either the TimeoutHandler sends its 503 Service Unavailable response first or the read/write deadline is reached first, allowing the handler to respond with a custom error message. The response would then be less deterministic. Could this be occurring or are there good ways to prevent this?

It might also not be necessary to prevent the deadlines from exceeding the timeout at all if the deadlines are reset anyways when the timeout is hit.

While we're in here, we should probably ensure that ResponseController.EnableFullDuplex works in a TimeoutHandler. Hijack should still return an unimplemented error. Flush should either return an unimplemented error or work correctly.

👍