Request timeout (WIP)

[zuiderkwast]

New request option timeout.

The timer ref is stored in the stream record in each protocol module (gun_http, gun_http2).

It's still untested, but you're welcome to have a look to see if it's the right approach at all. I'd appreciate some pointers.

I added request timeout for connect, but I don't know it's the way it should be. Also I'm not sure timeout is handled correctly in tunneled requests, but I hope it will be more obvious when I start writing some tests. (I'd be fine with not supporting request timeout for connect or in tunneled requests at all, since we don't need it, but I guess you want it to be there.)

Fixes: #317

Todo:

• Tests
• Docs
• Correct handling of connect and tunneled requests

[essen]

To make sure I understand the intent: this timeout triggers when the stream is still ongoing after a certain period of time, regardless of its current situation, without ever being reset? So this is more like a maximum life time of a stream than a timeout (since if I understand correctly it can drop streams that have data going in or out), is that correct?

Can you provide more context on the use case?

Should this be done on a per request basis instead of having a protocol option? (We can always handle Websocket over HTTP/2 specially if it's a protocol option.)

[zuiderkwast]

The context is in #317.

A server can exhaust client resources if some HTTP/2 requests hang indefinitely while other requests succeed normally. We just use simple HTTP/2 requests. No push, no websocket, no tunnels.

It doesn't matter really if it's an idle timeout per stream or a maximum life time per stream.

I put it in ReqOpts because it seemed to be protocol independent (at least independent of HTTP version), but it doesn't really matter either.

[zuiderkwast]

So this is more like a maximum life time of a stream than a timeout (since if I understand correctly it can drop streams that have data going in or out), is that correct?

Yes, correct, but I can change it. For CONNECT, it may not make that much sense. Perhaps we should stop the timer when CONNECT returns, i.e. when the tunnel is ready to use.

[essen]

We don't want a max lifetime for streams in Gun. I don't think it makes sense, if things are happening in the stream, as far as Gun is concerned, all is well. Max lifetime is a user application concern.

An idle timeout I think it makes sense to have. That timeout has to concern itself with stream activity rather than socket activity. It reacts on events pertaining to the request or the response directly. For example it would be reset on data being sent or received, but not on WINDOW_UPDATE or PRIORITY. Not sure about empty DATA frames, probably should be handled the same as WINDOW_UPDATE.

That idle timeout should have a generous value as default. I am thinking 5 minutes. The value should be configurable via protocol options and overridable on a per-stream basis via the request options.

When a request is created as a result of a server push, the server push should inherit the timeout value (a new timeout is started). A PUSH_PROMISE frame should reset the idle timeout of the originator stream, but after that point the originator stream and the pushed stream are handled separately.

When a request is a CONNECT request, there is no issue because the CONNECT stream technically end. In HTTP/1.1 the stream ends explicitly; in HTTP/2 the stream is converted to a tunnel stream. Either way that's when the request/response concluded and so the timeout should only cover this span.

When a request is upgraded to Websocket, the mechanism is basically the same as CONNECT and so the same applies.

Note that for HTTP/1.1 there is a special case: if the stream that times out is the current stream, we must close the connection. If the stream is a pipelined stream, we can act the same as if the stream was canceled. For HTTP/2 canceling the stream is the right approach.

[zuiderkwast]

Max lifetime is a user application concern.

Fair enough.

That idle timeout should have a generous value as default. I am thinking 5 minutes. The value should be configurable via protocol options and overridable on a per-stream basis via the request options.

How about inifinity as the default? Then we don't need to start a timer and it is backward compatible (long polling).

About adding configs on both levels, I'm not sure it's needed. What's the use case? We can have it only per stream, at least initially, and later we can add it per connection (protocol opts) if anyone ask for it?

[essen]

Configuration on both levels ensure that we can override this behavior if really long polling is desirable in some cases. Though I guess you're right that if it's a request option it isn't necessary at the protocol level. So let's go with only request options for now and default to 5 minutes idle timeout.

Really long polling (more than 5 minutes without an event) should be pretty rare so as long as we mark it as a potential incompatibility it should be OK. After that much time has passed you can't really know whether the stream is still alive or not anyway without poking the server.

If necessary we can be smart about the timers, by having a single periodic timer clean up expired streams. But the timer implementation of Erlang should be good enough to make that unnecessary. If it becomes necessary we can refactor.