go: net/http: no way of manipulating timeouts in Handler
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 Write
s.
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
.
About this issue
- Original URL
- State: closed
- Created 8 years ago
- Reactions: 213
- Comments: 78 (42 by maintainers)
Links to this issue
Commits related to this issue
- net/http: add Server.ReadHeaderTimeout, IdleTimeout, document WriteTimeout Updates #14204 Updates #16450 Updates #16100 Change-Id: Ic283bcec008a8e0bfbcfd8531d30fffe71052531 Reviewed-on: https://go-r... — committed to golang/go by bradfitz 8 years ago
- Experiment with SSE for session timeout — committed to NatLibFi/qvain-api by wvh 5 years ago
- Set some server http timeouts When using GoatCounter directly internet-facing it's liable to keep connections around for far too long, exhausting the max. number of open file descriptors, especially ... — committed to arp242/goatcounter by arp242 4 years ago
- Set some server http timeouts When using GoatCounter directly internet-facing it's liable to keep connections around for far too long, exhausting the max. number of open file descriptors, especially ... — committed to arp242/goatcounter by arp242 4 years ago
This issue needs attention.
There are many reasonable use cases, like different policies depending on authentication or varied types of requests. In my case, a single service is supposed to usually serve small responses, but sometimes after authentication and understandind the request, it turns out that the file to be served is huge and might take some legitimate user hours to download.
So you have to balance between:
It is hard to comprehend how there is no way to keep a legitimate connection going, while giving unauthenticated users a proper timeout. I cannot understand how people are able to overlook this and expose Go services to the internet relying on net/http.
Is there any acceptable workaround besides having a reverse proxy babysitting the Go service? Or another package to drop in place of net/http?
ResponseController will be in 1.20, with the ability to set read and write deadlines from within a Handler.
We ran into this issue on our project: https://github.com/upspin/upspin/issues/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
The shortest tl;dr is that there is no way to set timeouts based on the endpoint or on the client, but only server-wide, nor there is any way to update timeouts while serving a response based on the changing circumstances.
The reason one wants to set timeouts is to protect against clients going away or being intentionally slow. A reason one might want to exempt an endpoint is for example if that handler is serving streaming content or long downloads. A reason one might want to exempt a client is for example following authentication.
https://github.com/golang/go/issues/16100#issuecomment-285573480 is a good practical example. I feel like the first two paragraphs of my original report are also not solution-oriented.
This bug is jumping around a bit, so I’ve tried to summarize the use cases below. Please respond if I missed something important. I will keep this comment updated with the list of use cases.
For any request:
For any response:
@FiloSottile (author of this issue) wrote a great blogpost on cloudflare’s blog about this. It’s not linked anywhere here so I thought I’d include it for any interested readers.
https://blog.cloudflare.com/the-complete-guide-to-golang-net-http-timeouts/
My interest in this is being able to continuously update the timeout (to a few seconds in the future each time) on SSE server-sent-event streaming handlers while keeping deadlines for other regular non-streaming handlers much shorter. Hope to see some movement on this soon. EDIT: exactly like this: https://github.com/golang/go/issues/16100#issuecomment-405068548
I think this can be done with providing a custom
net.Listener
to(*http.Server).Serve
.That is embedding a
*net.TCPListener
and overwriting theAccept
method, which return a customnet.Conn
. The customnet.Conn
will embed a*net.TCPConn
and overwrite theWrite
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.
Hi Any updates ??
Any updates on this ?
I want to do this. I have an endpoint which exports a large file while others perform operations on smaller sized data. I would like to keep different timeouts for these two different endpoints.
Hi guys. I tried to implement a patch for this issue. I made 2 modifications on net/http in order to timeout slow Request.Body.Read() and RequestWriter.Write().
https://golang.org/cl/47390
Make http.Request.Body.Close() abort pending Read().
This is for timeout slow upload. For HTTP/2, requestBody.Close() have already been able to abort Read(). So, somehow I made HTTP/1.1 work like that.
Allow http.ResponseWriter upgrade to io.Closer to Close the connection/stream.
This is for slow download.
Now, we can unblock in-flight Read()Write()s by calling Close() concurrently on the body. I think that with those modifications, we can easily implement timeouts on Read()/Write() body like below. How do you think about this approach?
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.
I don’t have time to read and consider the discussion above. I just wanted to say I’d heavily argue against new optional interfaces. I think the existing ones were probably a mistake and I’d like to stop the bleeding.
@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: https://github.com/golang/go/issues/16958#issuecomment-244239789
You allude to your original problem here:
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?
#54136 proposes adding a new ResponseController type to address this issue.
Based on the gist here, I ended up with this hack/workaround
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@karaatanassov
I’m pretty sure it doesn’t 🙅 Your code only looks like it’s working because your calls to
w.Write
are writing very small chunks. You are effectively making use of socket buffers, meaningw.Write
is effectively non-blocking until the buffer is full. Try out this slightly modified version of your code (without the reader). Start it and run a slow reader against it (e.g.curl -Ns http://localhost:8080 | pv -q -L 1
). You’ll see a bunch of “writing” output, until the buffer is full. Then you’ll see your handler return, but thewriteResult
goroutine will remain active for as long as the slow reader keeps slowly reading. So this is unfortunately not a solution to the original problem, since the connection is still open.Also, @tv42’s comment is correct and should be reason enough to avoid this solution. Your code most definitely is using the
ResponseWriter
after the handler returns.But let’s try not to derail this issue any further. If you want, we can keep talking about this on the linked gist’s comments.
How about
(*http.Request)
or(*http.Request).Body
implementingSet*Deadline
?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?
@bradfitz @fraenkel @FiloSottile could any of you file a specific proposal from the above discussion in a new issue?
Hello any news ?
@bradfitz Thank you for your feedback!
I think introducing a new helper structure rather than adding new optional interfaces would be a good idea. It would enable us to extend the functionalities without breaking backward compatibility or having a lot of function-specific-interfaces.
I’m interested in your plan and I’d like to see more detail on it. But I’m not gonna rush you. I’m gonna stop working on my CLs until new proposal will come.
Also, if the new struct supports settings of per-Read/Write timeout like below, users don’t have to implement their own TimeoutReaders which I described above. I expect @tombergan would like this API).
Until Go supports something like this:
http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_read_timeout http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_send_timeout
if you need to read large requests or serve large responses (for example upload/download files), it is better to expose your Go application behind a reverse proxy like nginx
@karaatanassov Your code is buggy because
https://golang.org/pkg/net/http/#ResponseWriter
Hi Any updates ??
That’s an argument for why Close is an expedient solution, but not an argument for why Close is the right API. The Close solution forces callers to write little adapters, such as your timeoutReadCloser adapter or your context adapter. Why should callers have to go through so much trouble?
The contexts solution reuses a standard construct, contexts, which Go programmers should already be familiar with. There exist standard ways of manipulating contexts – no custom adapters needed. We may even want to define interfaces to
io
, such asio.ContextReader
, as well as standard adapters that convert a pair of(context.Context, io.ContextReader)
to anio.Reader
. For example, the response writers for HTTP/1 and HTTP/2 might implementio.ContextReader
as defined below:The only ugliness (to me) is that callers would need explicit casts, but the Close solution also has this problem. I guess this boils down to whether or not you believe the standard library should be more context-ified. This is a larger question that is worth raising beyond this issue, and I will do so after GopherCon this week.
@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.
(sorry if partly OT)
@karaatanassov you might need to add a LICENSE to your project, otherwise no one should be able to leverage your code as example
Pulling together a few comments over time here. Given the
ResponseWriter
is what currently let’s ushijack
the underlying connection, would it be sufficient to start havingnet/http
’s ResponseWriterimplement
io.Closerand a new
http.DeadlineController` with methods:This would be in line with the other interfaces already in use and implemented by the underlying
ResponseWriter
from the server.For a user wanting to implement Server-Side Events, they could do the following:
You could mix in
io.Closer
into that example or use therequest.Context
to signal the handler to stop the stream.I do also like @tombergan’s mention of contextified reads and writes as potentially new methods so as to avoid breaking the library.
With all that said, it feels like the right future approach is to keep functionality scoped to the
ResponseWriter
and not at a higher level (handler, router, or server) for the motivation of having the most flexibility and obvious/expected API.LGTM, although I would return an error (
Set[Read|Write]Deadline(time.Time) error
) so that we can communicate why the feature is not available.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.
@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
?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
@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.
Well … not in any elegant way. But I do have a workaround (unfortunately using an unsafe hack to support tls connections) which allow a monitoring go-routine to decide when to call Close() on the underlying net.Conn. It’s not ideal, but it works for, at least, HTTP/1.x
To conclude this there is no way to end requests with pending write on Linux and Windows. The workarounds I suggested work on MacOS only.
It seems a legitimate bug that http requests with pending write cannot be completed/cancelled. This makes go servers precarious for internet use. It is not only timeout that is missing.
Good point. The code technically is not using the
ResponseWriter
after the http handler has returned. Exactly the opposite it returns as to stop using it. I agree it is not the cleanest thing. It is one way to end response by return from the handler. So that is why I used it.A cleaner way could be to use
Request.WithContext
to create cancel-able request. That seems cleaner and indeed recommended in the http module.I will still keep the
ResponseWriter.Write()
in different go routine though as there is not non-blocking version of that.PS I have updated the example to use cancel-able Request and not return from the handler before the response is released. https://github.com/karaatanassov/go_http_write_timeout
@karaatanassov this is a bit off-topic, but just avoid problems for the people who find your snippet: unless I’m overlooking something, it unfortunately doesn’t really solve the underlying problem. It just “sweeps it under the rug”, so to speak. Your handler will return, but the
writeResult
goroutine will still be there for as long asw.Write
blocks (e.g. because of a slow reader). This means the underlying connection is still open, so resources are still being consumed by the client.For what it is worth I seem to have found a simple workaround. Idea is to use the response writer from a separate go routine that is linked with a channel to the http handler. This allows the handler to to return and close the request/response when slow consumer is detected or the connection breaks.
See https://github.com/karaatanassov/go_http_write_timeout/blob/989c390106c8344974b72d2207ff7de61357b6ac/main.go#L14 for example
In my observation the blocked
Write()
operation terminates 5 seconds after the http handler function returns.I am not sure if this is safe for production so will pay with it. But it seems the situation with this blocking
Write()
call with no timeout is not as desperate.My project also needs per-handler timeouts. So I can do something like
req.SetReadTimeout() req.SetWriteTimeout()
in handler after classifying incoming request.
nginx have the
send_timeout
option: https://nginx.org/en/docs/http/ngx_http_core_module.html#send_timeoutThis can be implemented with a new
Server.SendTimeout
field.Is http.ErrAbortHandler enough? Can we close this?
I’m not yet sure if I like using Close in that way. It’s certainly an expedient solution, but is it the right API?
If we were doing everything from scratch,
body.Read
andrw.Write
would take acontext
, which would naturally allow the caller to specify deadlines on each call. I would like to explicitly reject that approach before using @matope’s solution. Otherwise, we could end up in a situation like we’re in for Request cancellation, where there are three ways to cancel a request, two of which are deprecated (Transport.CancelRequest, Request.Cancel).It took me an awfully long time, but it finally clicked what didn’t sit right with the idea of making a return from ServeHTTP the way to break I/O: it overloads the
http.Handler
interface with a behaviour that won’t be guaranteed by any other user of the interface. Anything from tests to frameworks will not replicate the behaviour, and there will be no way for the code to know (which is the point of interface upgrades). This makes the weird inversion of control where you do the main work in a child goroutine especially bad, because if you start the I/O goroutine, then return from ServeHTTP expecting that to unblock the I/O goroutine, and it actually doesn’t, you leaked a goroutine.And anyway, I don’t think we should ever encourage starting a goroutine and losing track of it before knowing it returned.
I think we should go for some sort of interface upgrade to io.Closer, possibly in a way that works, or is easy to support, with ServeHTTP composition (aka middlewares).