Split RequestTimeout, ResponseTimeout, and KeepAliveTimeout into different timeouts

[ashleysommer]

This is a big change to the server backend of Sanic, and my largest Sanic contribution by far. There will need to be some discussion around this.

While investigating what seemed like a relatively simple problem in Issue #902 it was discovered that Sanic uses a single timeout (called RequestTimeout), which is handled by the connection_timeout() callback, and controlled by the 'REQUEST_TIMEOUT' config option, to cover three different scenarios:

1. The request took too long to come from the client,
2. The response took too long to come from our application,
3. We are using using a KeepAlive connection, and the KeepAlive timer runs out.

In all three of these scenarios, the server generates a 408 ("RequestTimeout") error and sends a 408 response to the client. While this is correct for case (1), it is incorrect behaviour for scenarios (2) and (3).

Even more confusting is if you look in the test_receive_timeout.py Test code, the test code injects a delay into the response, causing the response to timeout, then throws, catches, and delivers a 408: Request Timeout. This is confusing and wrong.

In the case of (2) when our application takes too long to generate a response to a request, we should use a ResponseTimeout, and send a 5xx error.

In thecase of (3), when the keepalive timeout expires, we should not send an error at all, just log it and move on.

To put it another way,

• A RequestTimeout is a Client error. It is caused by the client taking too long to send a request (after establishing a TCP connection). It gets a 4xx series error response, in this case 408.
• A ResponseTimeout is a Server error. It is our application doing something that is taking too long, so it should send a 5xx series error response. In this case a 503 is the closest match.
• A KeepAlive timeout is not an error. It is normal and expected that every connection will eventually time out. This is what Issue sanic.exceptions.RequestTimeout: Request Timeout #902 was about.

This PR has some big changes.
There are now three different timeouts tracked by the sanic backend server, each with their own configurable timeout values, their own default values, their own callback handlers, and their own logical flow.

1. RequestTimeout (error 408), which is a timeout that occurs between the point in time when a TCP connection is created, and the the point in time when the entire HTTP request is received. Default=60 seconds.
2. A ResponseTimeout (error 503), which is a timeout that occurs between the point in time when the server starts to process a request, and the point in time when the server delivers a response. Default=60 seconds.
3. A KeepAliveTimeout (not an error), which is a timeout that occurs between the point in time when the server delivers a response, and the point in time when a new request is received on the same TCP connection. Default = 5 seconds.

Fixes #902

[ashleysommer]

In this PR, all three timeouts now have their own group of tests.

There have been no performance benchmarks done yet.

[ashleysommer]

Ok, while the tests are passing on my machine, they are not passing on Travis.
Will need to do some more work on the tests.

[ashleysommer]

Woo! passing now :)

[r0fls]

@ashleysommer sorry still reviewing this. Looks great though.

[yunstanford]

Is there any reason we need this ? can we use test_client fixture in pytest-sanic ? https://github.com/yunstanford/pytest-sanic, it'd be better to maintain.

[ashleysommer]

This is here because I needed a custom version of the SanicTestClient that could:

1. Honor a keepalive header, and store the connection in the client for reuse.
2. Reuse the existing TCP connection when making a subsequent request, if keepalive was set on the first request.
3. Raise an error if we got a new connection, when we were expecting to reuse the old connection.

These customisations to the SanicTestClient are very specific to this particular test (the KeepAliveTimeout test)

I haven't looked, but I don't think the test_client fixture in pytest-sanic has the ability to be customised in this manner.

[yunstanford]

No, you can do them all, by simply passing your ReuseableTCPConnector as param to test_client.
https://github.com/yunstanford/pytest-sanic/blob/master/pytest_sanic/utils.py#L183

[r0fls]

I'm going to go ahead and merge this now and we can improve it later, if needed. @yunstanford if you have time to make a PR with the pytest-sanic module that'd be nice. Thanks @ashleysommer!

[ashleysommer]

@r0fls Ok, thanks for merging.

To be honest, I was expecting more discussion around this before merging, specifically about my choice of default values for the config variables, and the logic around deciding when each timeout is triggered. But I guess those things can be discussed in other PRs or Issues now.

I don't know how many people are using/testing sanic@master, but I expect that some people might notice a change in sanic server behavior, particularly if they were relying on the specific nuances of the previous behavior. There may be some new github Issues popping up to around this, as it may appear to some as bugs/regressions.

Another thing I thought of is the documentation. The docs for the REQUEST_TIMEOUT config variable will need to be changed, plus docs for RESPONSE_TIMEOUT and KEEP_ALIVE_TIMEOUT will need to be written. I will open a PR with those docs changes in the next few days, when I get a chance.

[r0fls]

@ashleysommer ok thanks. For the record, if a PR is not complete when created, and you think it requires feedback or more work (e.g. docs additions), then you can mark it as WIP to indicate that.