Change https://go.dev/cl/420174 mentions this issue: net/http: work in progress per-request timeouts

Thanks for the proposal.

IIUC this will unblock removing timeoutHandler in kubernetes completely which has been a source of many hard to debug races in the past (kubernetes/kubernetes#105884).

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

If people want to understand a use case for this, at work we typically set a higher timeout duration for writes than reads (via http method.) The idea being that writes should have a higher likelihood of succeeding, and a faster timeout for reads forces engineers to work harder to make more efficient code. We haven’t implemented this in our go code because we couldn’t figure out how, the we do in other languages.

Does anyone object to adding this API?

@neild @rsc Just to clarify if I understand this correctly. This proposal would make the timeouts only possible per separate http.Handler, right?
I am trying to understand if this will be usable for a http proxy application that wants to set these timeouts per route, such that developers with different applications can change for their route these timeouts. Right now a single route has not a separate http.Handler in our case. As far as I understand we would need to create a http.Handler per route to isolate the timeout handling.

Routes in this case are not static and can be created, changed, deleted every ~3 seconds. The proxy serves a variety of applications next to each other.
If we wrap the generic http.Handler with a ResponseController and set SetReadDeadline/SetWriteDeadline, then the deadline would change for the full proxy instance as far as I understand or we would need to create http.Handler per route or per group of different timeouts.

I hope my question is understandable. :)

This proposal is for a mechanism to permit a handler to adjust the read and write deadlines for a request after the handler has been called.

Sample usage would be something like:

http.HandleFunc("/foo", func(w http.ResponseWriter, r *http.Request) {
  ctl := http.NewResponseController(w, r)
  ctl.SetWriteDeadline(time.Now().Add(1 * time.Minute)) // sets deadline for this request only
  fmt.Fprintln(w, "Hello, world.") // this write has a 1-minute timeout
})

@neild thanks! That was also my understanding, thanks for the clarification and example.

Hello, also need this for streaming connections, a couple more questions to clarify:

1. This will transparently work with both http/1.1 and http/2 connections right?
2. What about http/3 servers based on https://github.com/lucas-clemente/quic-go?

This will transparently work with both http/1.1 and http/2 connections right?

Yes.

What about http/3 servers based on https://github.com/lucas-clemente/quic-go?

As proposed, this only works with ResponseWriters provided by net/http, or ones which wrap one.

My first thought was that the initial version of this proposal doesn't need to support third-party ResponseWriters, but on consideration I think that perhaps we should: We need some mechanism for golang.org/x/net/http2 to provide a ResponseController for its ResponseWriter. Every way of doing this I can think of will be usable by third-party packages as well. So we might as well document that mechanism and support it, since someone is going to rely on it anyway.

Changes to the proposal:

• Drop the *Request parameter to NewResponseController. We don't need it, and I haven't been able to think of anything that would require it in the future.
• If a third-party ResponseWriter passed to NewResponseController implements a Flush, Hijack, etc. method, then the ResponseController will call them.

// NewResponseController creates a ResponseController for a request.
//
// The ResponseWriter should be the original value passed to the Handler.ServeHTTP method,
// or have an Unwrap() method returning the original ResponseWriter.
//
// If the ResponseWriter implements any of the following methods, the ResponseController will
// call them as appropriate:
//
//    Flush()
//    Flush() error // alternative Flush returning an error
//    Hijack() (net.Conn, *bufio.ReadWriter, error)
//    SetReadDeadline(deadline time.Time)
//    SetWriteDeadline(deadline time.Time)
func NewResponseController(rw ResponseWriter) *ResponseController

@neild I think this helps also my use case :)

Change https://go.dev/cl/436890 mentions this issue: net/http: alternate work in progress per-request timeouts

While working through a draft implementation of this, I came across a somewhat different approach to integrating non-net/http implementations (including x/net/http2) with ResponseController. A demonstration is in https://go.dev/cl/436890.

In this approach, ResponseController is an interface:

type ResponseController interface {
        Hijacker
        Flush() error
        SetReadDeadline(deadline time.Time) error
        SetWriteDeadline(deadline time.Time) error
        responseController() // can only be implemented within net/http
}

A ResponseWriter may have a ResponseController method returning the response controller. NewResponseController is a type assertion to look for a ResponseController method and call it. If there is no ResponseController method, NewResponseController returns a ResponseController that returns an "unimplemented" error for every method.

func NewResponseController(rw ResponseWriter) ResponseController {
        if rc, ok := rw.(interface{ ResponseController() ResponseController }); ok {
                return rc.ResponseController()
        }
        return unimplementedResponseController{}
}

We want to be able to add new methods to ResponseController in the future without breaking existing implementations. To ensure we can do this, the ResponseController interface contains an unexported method. Implementations outside net/http (including the one in x/net) embed the result of NewResponseController(nil), which ensures that they return an appropriate error from any methods they don't implement.

The advantage to this approach is less indirection: There's no need to pass each ResponseController method call through an intermediate function (the approach I was taking in https://go.dev/cl/420174). When we add a new method to ResponseController, we don't need to add a new corresponding magic method to ResponseWriter; we just add it to unimplementedResponseController and existing implementations automatically acquire reasonable default behavior.

A disadvantage is that the pattern of embedding a default implementation of an interface with unexported methods is not commonly used in the standard library. However, this only affects people writing ResponseController implementations (a very specialized and uncommon task); users of the ResponseController API don't need to worry about this at all.

@neild, it sounds like there are two possible APIs here. Which one do you suggest? And does anyone else have any opinions on which one we use?

After discussion with @bradfitz, we think the original proposal with the update in #54136 (comment) is the right approach. Making ResponseController an interface is an interesting idea, but the benefits (if any) are minor and the mechanisms used to make it work are non-obvious.

Does anyone object to the API in #54136 (comment) ?

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

Excited to see progress on #16100!

One thing that I don't see specified is whether a call to SetReadDeadline or SetWriteDeadline overrides the ReadTimeout or WriteTimeout. I would suggest yes, so that servers could be configured with safe defaults, and then the timeouts only relaxed after checking auth, looking at the path, etc.

Also, ReadHeaderTimeout covers the entire period before ResponseController is available, correct?

Change https://go.dev/cl/449935 mentions this issue: all: update vendored golang.org/x/net to v0.2.0

Change https://go.dev/cl/450515 mentions this issue: doc/go1.20: add release notes for net/http and net/http/httputil

This will be in 1.20.

Is it possible to somehow make deadlines be set also for individual Read and Write operations on the net.Conn underlying the request/response session?

At my $dayjob, we use long-lived HTTP connections (with chunked encoding) to transfer data ("soft-realtime"). These connections only end when either the client loses interest in receiving the data or the upstream shuts down, so a deadline for the full request has no sense—as basically there is no "end".
What we want, instead, is to be able to discover a data stream be stalled for too long: there are keepalives sent in the stream, so basically if Read is stuck for, like, ×2 times the keepalive period, the client should close the request and retry. Write has the same property: to be stuck in this call for too long means we should trash this connection and have its client to retry.

To implement such policy, we have a custom type which wraps the net.Conn and calls SetDeadline before each Write and Read (and resets it afterwards). This is certainly a rather hacky way to achieve what we're after.
As I can see we're not alone with such requirements.

So, can there be thought a way to implement such finer-grained controls?

The http2.Transport has a WriteByteTimeout setting which sets the maximum amount of time to wait on any given write. I could see extending that to the HTTP/2 server and the HTTP/1 transport and server. We'd want to think through how that interacts with HTTP/2 health checks.

You could also useResponseController.SetReadDeadline and ResponseController.SetWriteDeadline to limit the time for reading/writing a single chunk on a long-lived connection, resetting the deadline after every read/write.

The http2.Transport has a WriteByteTimeout setting which sets the maximum amount of time to wait on any given write. I could see extending that to the HTTP/2 server and the HTTP/1 transport and server. We'd want to think through how that interacts with HTTP/2 health checks.

Sorry, I forgot to state that we're currently using HTTP 1.1.
So yes, I would love to see such support for both stacks, if possible ❤️

Is it possible to somehow make deadlines be set also for individual Read and Write operations on the net.Conn underlying the request/response session?

At my $dayjob, we use long-lived HTTP connections (with chunked encoding) to transfer data ("soft-realtime"). These connections only end when either the client loses interest in receiving the data or the upstream shuts down, so a deadline for the full request has no sense—as basically there is no "end". What we want, instead, is to be able to discover a data stream be stalled for too long: there are keepalives sent in the stream, so basically if Read is stuck for, like, ×2 times the keepalive period, the client should close the request and retry. Write has the same property: to be stuck in this call for too long means we should trash this connection and have its client to retry.

To implement such policy, we have a custom type which wraps the net.Conn and calls SetDeadline before each Write and Read (and resets it afterwards). This is certainly a rather hacky way to achieve what we're after. As I can see we're not alone with such requirements.

the purpose of the above code is to extend the deadlines if we read/write a certain amount of bytes in a given time. This is required to protect long lived connections from Slowloris style attacks. I haven't tested the ResponseController yet but I think we can implement the same logic using it in combination with reader/writer wrappers. I'm not sure it's appropriate to simplify this use case directly into stdlib, but that would be great

So, can there be thought a way to implement such finer-grained controls?

This proposal won't affect the idle timeout between the requests (http.Transport.IdleConnTimeout), right? I have a usecase, where some connections need a different, longer idle timeout.

Even if I use the ResponseController, say something like that:

# .. in a handler ..

if err := rc.SetReadDeadline(time.Now().Add(longDeadline)); err != nil {
	logger.Errorf("Failed to set the timeout ...", ..)
}

This is then overwritten by the idleTimeout() property [src]:

..
		serverHandler{c.server}.ServeHTTP(w, w.req)       # <-- ResponseController sets a deadline here
..
		if d := c.server.idleTimeout(); d != 0 {
			c.rwc.SetReadDeadline(time.Now().Add(d))  # <-- deadline gets overwritten here
		} else {
			c.rwc.SetReadDeadline(time.Time{})
		}
..

Do I get this right? Are there any workarounds / patterns to have a different per-connection idle timeout between the requests?

This proposal won't affect the idle timeout between the requests (http.Transport.IdleConnTimeout), right?

Correct.

net/http doesn't really have any facilities for treating a connection differently based on a request received on it. (Aside from hijacking the conn entirely and removing it from the net/http package's control.)

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