net/http: no way of manipulating timeouts in Handler

[FiloSottile]

A Handler has no way of changing the underlying connection Deadline, since it has no access to the net.Conn (except by maintaining a map from RemoteAddr to net.Conn via Server.ConnState, but it's more than anyone should need to do). Moreover, it can't implement a timeout itself because the Close method of the ResponseWriter implementation is not documented to unblock concurrent Writes.

This means that if the server has a WriteTimeout, the connection has a definite lifespan, and streaming is impossible. So servers with any streaming endpoints are forced not to implement timeouts at all on the entire Server.

A possible solution might be to expose the net.Conn in the Context. Another could be to allow interface upgrades to the SetDeadline methods on ResponseWriter. Yet another would be to make (*response).Close unblock (*response).Write.

[elithrar]

Given that we store the *http.Server in the request context, making net.Conn available in the context via (e.g.) ConnContextKey could be an option. This could be opt-in via a field on the http.Server as stuffing the request context with things by default is not ideal.

[noblehng]

I think this can be done with providing a custom net.Listener to (*http.Server).Serve.

That is embedding a *net.TCPListener and overwriting the Accept method, which return a custom net.Conn. The custom net.Conn will embed a *net.TCPConn and overwrite the Write method.

The overwritten Write method could reset the write deadline on every write, or use a atomic counter to reset the write deadline on some numbers/bytes of consecutively write. But for truly on demand write deadline resetting, one still need some way to do that on the higer level handler side.

Since a http/2 connection can do multiplexing, it would be helpful to have a set of timeouts for individual stream. When a stream hang on client, we could use those timeouts to release resources associated with that stream, this is not possible with setting deadlines on the lower level underlying connection.

[bradfitz]

@FiloSottile, we won't be exposing net.Conn to handlers, or let users explicitly set deadlines on conns. All public APIs need to consider both HTTP/1 and HTTP/2.

You propose many solutions, but I'd like to get a clear statement of the problem first. Even the title of this bug seems like a description of a lack of solution, rather than a problem that's not solvable.

I agree that the WriteTimeout is ill-specified. See also my comment about ReadTimeout here: #16958 (comment)

You allude to your original problem here:

So servers with any streaming endpoints are forced not to implement timeouts at all on the entire Server.

So you have an infinite stream, and you want to forcibly abort that stream if the user isn't reading fast enough? In HTTP/1, that means closing the TCP connection. In HTTP/2, that means sending a RST_STREAM.

I guess we need to define WriteTimeout before we make progress on this bug.

What do you think WriteTimeout should mean?

[thincal]

Why not provide a timeout version of Read/Write ? Is there any reason ?

Request.Body.TimedRead(p []byte, timeout time.Duration)
Request.Body.TimedWrite(p []byte, timeout time.Duration)

My concrete case is:

case1 about read

client send data via. a POST API provided by server, but if the network disconnected during the server read request data via. request.Body.Read, the server will be blocked at Body.Read forever.

case2 about write

similar that it's server write data back to client.

So I need a timeout mechanism that server can be unblocked from this Read/Write.
This is very intuitive requirement, might be there are other solution already ?

Thanks.

[bradfitz]

What if the mechanism to abort blocked read or write I/O is to just exit from the ServeHTTP function?

So then handlers can do their own timers before reads/writes and cancel or extend them as necessary. That implies that all such reads & writes would need to be done in a separate goroutine. And then if they're too slow and the timer fires before the hander's I/O goroutine completes, just return from ServeHTTP and we'd either kill the TCP connection (for http1) or do a RST_STREAM (for http2).

I suppose then the problem is there's a small window where the I/O might've completed just in the window between the timer firing and the ServeHTTP code exiting, so then once the http package sees ServeHTTP is done, it may not see any calls still in Read or Write, and might then not fail the connection as intended.

That suggests we need some sort of explicit means of aborting a response. http1 has that with Hijacker.

I've recommended in the past that people just panic, since the http package has that weird feature where it recovers panics.

Would that work? Do all your I/O in separate goroutines, and panic if they take too long?

We might get data races with http1 and/or http2 with that, but I could probably make it work if you like the idea.

[gopherbot]

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

[bradfitz]

Deferring to Go 1.9 due to lack of reply.

[c4milo]

@bradfitz I faced a similar problem when I started with Go. I had a service in Node using HTTP 1.1 with chunked transfer encoding, it implemented bidirectional streaming over HTTP 1.1. On each write or read, the deadline/timeout would get reset in order for the connection to remain opened. It worked on Node.js which uses libuv. This predated websockets and avoided long polling. It is a little known technique but is somewhat mentioned in https://tools.ietf.org/rfc/rfc6202.txt (numeral 3). I also remember Twitter's streaming API using it. Anyway, migrating my Node service to Go was not possible because of the use of absolute timeouts/deadlines.

So, I guess @FiloSottile is referring to a similar case. At least in my particular scenario, what I wanted to do was to be able to reset the connection's write and read timeout/deadline so that the connection remained open but still got closed if it became idle.

[drakkan]

A timeout that restart on each write or read would be useful for my use case too and if implemented I could rewrite in go a legacy c++ application.

If the timeout could be set per request and not only globally would be a good plus

[FiloSottile]

@bradfitz Sorry for not following up on this one, it turned out to be more nuanced than I thought (HTTP/2, brain, HTTP/2 exists) and didn't have the time to look at it further.

Doing I/O in a goroutine instead of the timer sounds upside-down, but I don't have specific points to make against it. What would make more sense to me would be a way to cancel the whole thing, which would make Body.Read/ResponseWriter.Write return an error that would then be handled normally.

Question, without such a mechanism, how is the user supposed to timeout a read when using ReadHeaderTimeout?

    // ReadHeaderTimeout is the amount of time allowed to read
    // request headers. The connection's read deadline is reset
    // after reading the headers and the Handler can decide what
    // is considered too slow for the body.

[FiloSottile]

One possible solution would be for Request.Body to be documented to allow Close concurrent with Read, and not to panic on double Close, and for ResponseWriter to be upgradeable to io.Closer.

[peter-mogensen]

For what it's worth ... I think we have a related use case. (only for reading requests and not for writing)

We have some slow clients doing large PUT requests - sometime on unstable connections which dies.
But we would like to allow these PUT requests as long as there is actually progress and Read() returns data. Preferably only on the endpoints where that should be allowed.

Currently, though we provide a net.Listener/net.Conn which sets Deadline on every Read/Write ... but that seems not a viable solution since it would interfere with timeouts set by net/http.Server.

[peter-mogensen]

Would that work? Do all your I/O in separate goroutines, and panic if they take too long?

The problem is defining what is "too long".
Often what you want is not to kill long running IO in absolute terms, but to kill "too slow" connections.
I don't want to kill a long running PUT request as long as it's actually transferring data. But I would kill a request only sending 1 byte/second.

These demands for IO activity should (for HTTP) only be enforced during ConnState stateActive (and stateNew). It would be OK for a connection to not have any IO activity in stateIdle.

I've been doing some experiments setting deadlines on connections - which is defeated by tls.Conn hiding the underlying connection object from external access.
Also trying to have a reaper go-routine Close connections with no IO activity. ... which becomes equally messy, although not impossible. (**)

Being able to set demands for IO activity during HTTP stateActive - or a more general way to do this for net.Conn without overwriting deadlines set by other API and regardless of whether the conn object is wrapped in TLS. - would be nice :)

** PS: ...much of the complexity comes from not being able to access the underlying connection in a crypto/tls.Conn object. I can understand why not exposing the connection to a Handler, but in the ConnState callback, is there any reason not to allow getting the underlying connection for a tls.Conn?

[pwaller]

Just to throw in my 2¢, I also have personally hit the need to set timeouts on infinite connections, and have met someone at a go meetup who independently brought up the same problem.

In terms of API, why can't the underlying ResponseWriter implement Set{Read,Write}Deadline? It seems to me that they could happily do the write thing of sending STREAM_RST for http2. Just because it happens to implement the same API you'd use to timeout TCP connections doesn't mean it has to time out the TCP connection itself if there is another layer that can be used.

I agree with @FiloSottile's comment about the separate goroutine seeming upside-down. Though I sort of like it, it uses go's primitives in a clever non-obvious way which makes me feel nervous and seems like it would be hard to document.

[peter-mogensen]

Having ResponseWriter have Set*Deadline() would only solve to OP problem of writing data to the client. - not the problem of stopping long running PUT which don't really transfer data at any satisfying rate.

[pwaller]

How about (*http.Request) or (*http.Request).Body implementing Set*Deadline?

[peter-mogensen]

If that can be implemented without interfering with other deadlines, like ReadTimeout, then it would be interesting to try.

[yonderblue]

@bradfitz Is it expected with the current master of the http2 pkg that when panicing out of a handler and a left behind goroutine blocked on a body read or response writer write, for a panic to occur that is internal to the http2 pkg? (existing TimeoutHandler would have same issue). Or is the behavior your referring to what would be made safe if that is the chosen route, but doesn't exist today?

[adg]

We ran into this issue on our project: upspin/upspin#313

We had our timeouts set quite low, as per @FiloSottile's recommendations but our users regularly send 1MB POST requests, which not all connections can deliver in a short time.

We would like to be able to set the timeout after reading the request headers. This would let us increase the timeouts for authenticated users only (we don't mind if our own users want to DoS us; at least we'll know who they are), but give unauthenticated users a very short timeout.

cc @robpike

[drakkan]

Based on the gist here, I ended up with this hack/workaround

type listener struct {
	net.Listener
	ReadTimeout  time.Duration
	WriteTimeout time.Duration
}

func (l *listener) Accept() (net.Conn, error) {
	c, err := l.Listener.Accept()
	if err != nil {
		return nil, err
	}
	tc := &Conn{
		Conn:                     c,
		ReadTimeout:              l.ReadTimeout,
		WriteTimeout:             l.WriteTimeout,
		ReadThreshold:            int32((l.ReadTimeout * 1024) / time.Second),
		WriteThreshold:           int32((l.WriteTimeout * 1024) / time.Second),
		BytesReadFromDeadline:    0,
		BytesWrittenFromDeadline: 0,
	}
	return tc, nil
}

// Conn wraps a net.Conn, and sets a deadline for every read
// and write operation.
type Conn struct {
	net.Conn
	ReadTimeout              time.Duration
	WriteTimeout             time.Duration
	ReadThreshold            int32
	WriteThreshold           int32
	BytesReadFromDeadline    int32
	BytesWrittenFromDeadline int32
}

func (c *Conn) Read(b []byte) (n int, err error) {
	if atomic.LoadInt32(&c.BytesReadFromDeadline) > c.ReadThreshold {
		atomic.StoreInt32(&c.BytesReadFromDeadline, 0)
		// we set both read and write deadlines here otherwise after the request
		// is read writing the response fails with an i/o timeout error
		err = c.Conn.SetDeadline(time.Now().Add(c.ReadTimeout))
		if err != nil {
			return 0, err
		}
	}
	n, err = c.Conn.Read(b)
	atomic.AddInt32(&c.BytesReadFromDeadline, int32(n))
	return
}

func (c *Conn) Write(b []byte) (n int, err error) {
	if atomic.LoadInt32(&c.BytesWrittenFromDeadline) > c.WriteThreshold {
		atomic.StoreInt32(&c.BytesWrittenFromDeadline, 0)
		// we extend the read deadline too, not sure it's necessary,
		// but it doesn't hurt
		err = c.Conn.SetDeadline(time.Now().Add(c.WriteTimeout))
		if err != nil {
			return
		}
	}
	n, err = c.Conn.Write(b)
	atomic.AddInt32(&c.BytesWrittenFromDeadline, int32(n))
	return
}

func newListener(network, addr string, readTimeout, writeTimeout time.Duration) (net.Listener, error) {
	l, err := net.Listen(network, addr)
	if err != nil {
		return nil, err
	}

	tl := &listener{
		Listener:     l,
		ReadTimeout:  readTimeout,
		WriteTimeout: writeTimeout,
	}
	return tl, nil
}

For my use case I use 60 seconds for http.Server and listener Read/Write timeouts. This way slowloris is not more an issue, hope this can help others and a that a proper solution will be included in http.Server directly. Please note that http.Server sets read and write deadlines internally so this workaround could break in future

[doveq]

Hi Any updates ??

[gwenaskell]

@drakkan Hi, thanks for the snippet. Just a remark: wrapping the net.Conn objet as you do will prevent the underlying TCP or TLS connection's ReadFrom method from being promoted (the method is not exposed by the net.Conn interface but can be accessed with interface casting using io.ReaderFrom).
This breaks an optimization made for files (*os.File) transfers through io.Copy(w, file), which uses the net package's sendFile function that uses system calls to minimize copying.
(the call stack is basically: io.Copy --> http.ResponseWriter.(io.ReaderFrom).ReadFrom --> net.Conn.(io.ReaderFrom).ReadFrom --> net.sendFile).

Since we only use TCP or TLS connections here, it should be fine to directly implement ReadFrom on your wrapper conn.

Of course, using ReadFrom bypasses the iterative deadline update that you have here, since it blocks until the whole file has been transferred. Fortunately, it is possible to chunk the file transfer by wrapping the *os.File into a *io.LimitedReader to read the file portion by portion, which is also supported by sendFile.

[gwenaskell]

Just adding a few comments to my previous message:

--> sendFile is only available for the http/1 library, so this approach does not break any optimization when using http2

--> controlling timeouts on the connection is probably not suitable for http/2, as this should be done on the stream.

[FiloSottile]

#54136 proposes adding a new ResponseController type to address this issue.

[neild]

ResponseController will be in 1.20, with the ability to set read and write deadlines from within a Handler.