Improve client timeouts

[danielrobbins]

Long story short

Currently, aiohttp imposes a default 5 minute timeout on client HTTP requests, after which the connection will be aborted. While deemed a useful feature by some, it does mean that without special workarounds, aiohttp by default violates RFC 2616 section 9.3 ("GET") which is likely to be unexpected behavior for developers who expect an RFC-compliant HTTP implementation.

In summary: the most straightforward interpretation of RFC 2616 section 9.3 ("GET") is that under normal conditions, and without any explicit attempts to cancel the connection, the client HTTP request is supposed to run until the full entity identified by the Request-URI has been transmitted to the client. By default, this may not occur, and in fact under many circumstances, will not occur.

Expected behaviour

An aiohttp client should, by default, hold the connection open indefinitely, as long as the underlying TCP connection is active, and allow the full entity identified by the Request-URI to be transmitted as per the RFC.

aiohttp should provide the ability for a client to abort a connection at any point, but this should not be the default behavior, since it is not the default behavior of HTTP, and aiohttp is supposed to implement HTTP.

Rationale

RFC 2616 states that "The GET method means retrieve whatever information (in the form of an
entity) is identified by the Request-URI." It does not specify that there is a time limit imposed upon this exchange of information. The entity is supposed to be returned to the client in its entirety. And in turn, the client is supposed to retrieve the entity, in its entirety. This is supposed to occur regardless of the amount of time required to send the data over the network. In other words, assuming a stable network, this is the intended behavior of HTTP.

There are only two exceptions to this behavior mentioned to this in the RFC: the conditional GET and the partial GET (Range: header). Unless these features are used, the intended behavior of the client is to retrieve the remote resource in its entirety, and for the server to send the resource in its entirety.

Again, the HTTP protocol does not impose any time restriction on the completion of an HTTP request. In fact, it is perfectly acceptable to an HTTP request to last more than 5 minutes -- even hours or days. This behavior is actually very common when transferring large amounts of data over a single HTTP request over a relatively slow connection.

But what's the problem of the 5-minute default, since it can be changed? The problem is actually quite significant. It means that aiohttp, by default, is not fully RFC-compliant unless special steps are taken by the developer. Since aiohttp is advertised as implementing the HTTP protocol, it should attempt to do so in the most RFC-compliant way possible, and not impose additional default behaviors that are not specified in the RFC. To do otherwise is counter-intuitive and violates community understandings of the behavior of HTTP under normal conditions. As such, the 5-minute timeout is not a benefit but rather an unnecessary complication and deviation from expected behavior.

[danielrobbins]

I will also add a less formal, but polite comment -- for anyone who thinks that the "abort after 5 minute" default is a good idea. For anyone who thinks this is a good idea, I ask if you have ever downloaded a big file? Do you not realize that you probably used HTTP for this, and it probably took over 5 minutes? The default behavior of aiohttp is to say "any download that takes over 5 minutes is probably invalid and should be aborted by the client by default." I have downloaded so many files over HTTP that have taken over 5 minutes in my lifetime -- thousands. Maybe in how you use HTTP, a 5-minute request seems wrong -- in the context of Web pages, maybe it would be "wrong", but HTTP is used for so many things, like downloads. I assure you, there is nothing wrong with such requests, in fact likely hundreds of thousands of such requests happen on the Internet every day, all fully RFC-compliant. Do not make it necessary to take special steps with aiohttp to unlock the "full" features of HTTP.

[asvetlov]

I don't think that RFC standard (neither old RFC 2616 no new RFC 7231-7240) specify timeout processing at all. Timeout questions are just out of scope of the HTTP RFC. There is no statement for fetching forever behavior as well).

On other hand completely disabling all timeouts can be considered as security issue: if a server sends 1 byte per hour without breaking a connection fetching such resource effectively hangs an executed task.
If the server calls another (micro-)server by REST or GraphQL protocol very long timeouts should be considered as connection-lost signals, otherwise deny-of-service attack can be mastered easy.

Almost every HTTP library supports timeouts configuring in API. HTTP is TCP based protocol, in turn TCP can disconnect a peer by timeout (there are several TCP options for it: SO_KEEPALIVE and TCP_KEEP* family). The problem is that options are cumbersome a little and not cross-platform.
That's why timeouts are usually calculated on application layer.

4. User should be aware about network timeouts when making HTTP requests. Documentation should be very expressive in this question. Timeouts should be mentioned in quickstart and explained in advanced in details.

5. Timeout event is pretty visible for user, explicit timeout exception is thrown (at least I believe aiohttp does it. If somewhere we've missed something -- it should be fixed).

[asvetlov]

@danielrobbins I agree that for downloading files current default timeout policy is doesn't make sense. We should document it at least and provide suggestions for large file downloading.

[danielrobbins]

@asvetlov there is no problem supporting the ability to have timeouts; in fact, this is desired. I believe there are two reasonable options. Option 1 is no client timeout by default, which allows for aiohttp to handle all potential use cases by default. The other option is an explicit client timeout behavior, where a user must define a timeout for every connection so that nothing is ambiguous.

Regarding the potential for abuse, a correctly-written server will drop a client connection after a certain timeout. To be abused, special techniques like slowloris need to be used to keep the server waiting for the HTTP request to complete, and well-written servers will guard against this too. And all Web servers generally have a configured "max upload" or have control over the resources that can be requested. So I view this as a non-issue when it comes to abuse -- it is the responsibility of the server to protect against it being abused. The tools simply need to be present on the server side to properly handle this, and this can be addressed in the aiohttp server documentation so that robust server examples are presented.

This takes the burden away from the client side, which can never be trusted, since you cannot control clients, and having the default timeout of 5 minutes can easily be bypassed by any malicious person anyway. So it just hurts the nice people who are trying to download large files, and actually does nothing to protect anyone from malicious behavior because bad people will do bad things and easily work around whatever measures you have in place.

[samuelcolvin]

@danielrobbins as you could probably have guessed I have downloaded files which take more than 5 minutes in the past, I'm well aware it's over http.

The insinuation that I didn't know that together with your comments on #2249 are not necessary and more to the point, not an effective way of arguing for a change in aiohttp.

My take on this

Docs

The use of async with async_timeout.timeout(10): in the client example at the top of the aiohttp docs is unnecessary and misleading - it indicates you need to use the timeout context which you don't.

Since some other libraries don't have a default timeout we should be very explicit about the fact that aiohttp has a timeout and how to override it.

Not strictly docs, but: the timeout exception is currently very unhelpful, just:

...
  File "/home/samuel/code/mithra/env/lib/python3.6/site-packages/aiohttp/helpers.py", line 661, in __exit__
    raise asyncio.TimeoutError from None
concurrent.futures._base.TimeoutError

This timeout should have a message saying "request read reached it's timeout of 300s and was cancelled, use read_timeout to override this" or something.

The default timeout

While of course http requests come in all shapes and sizes and aren't elegantly grouped into different types, you could roughly categorize http requests into two sorts:

1. pseudo instant (to a human) website requests and also webhooks type requests and most restful api requests. These requests generally send <1MB and receive less than <10MB. Many systems (eg. Heroku) limit requests like this to 30seconds.
2. "Data downloads" where an end user might expect a progress bar. These often consist of binrary data, can be of arbitrary size and should have no timeout, a very large timeout or custom timeout logic set by the application developer.

With this thinking, the use of 300s timeout is an odd choice. For me a 30s default timeout would make more sense, this would also mean unexpected failures happened more quickly and would therefore be more likely to be found quickly.

asyncio task exceptions confuse people

My suspicion is that developers new to asyncio are often confused by the way that exceptions inside tasks aren't raised or shown anywhere - they're "hidden" and only raised when task.result() is called. This still get's me occasionally: "what's happened to that task? Oh, now I've got the result and an error which occurred an hour ago has now blown up".

While this is a python feature and not specific to aiohttp, perhaps we could better guard against this confusion using warnings or logging on client requests?

[danielrobbins]

@samuelcolvin I think the ideas you shared above are all good. 300 seconds is indeed an unfortunate default as it is often creates an "edge case" that will cause a few requests to die mysteriously.

I would also just reiterate that I think it's the responsibility of the server to set policy for what kinds of HTTP requests are acceptable, and enforce this policy, and I don't think the client default really helps anyone. At least not at 300s. If I had to vote for a default value (rather than my first choice of removing it), I would be inclined to suggest 5 seconds.

Also very good are your comments about having the TimeoutError be much more detailed regarding the fact that it is in fact an aiohttp internal timeout setting and not some kind of network-based timeout related to the HTTP protocol.

[hubo1016]

There should not be a default overall timeout. It should be a "read timeout" instead. As long As the server is sending data, the connection should not be aborted. There are streaming requests that DO NOT END AT ALL. For example, docker has an interface to retrieve log from a running container. If the container is not stopped, the request is not ended.

A default read timeout is more useful, because not all connection problems are detectable. In some situations, the connection is broken in the other side, but the FIN packet never arrives the other side, then the socket is still open on the other side, causes the client to wait indefinitely.
Even Though users must have choices to disable it.

[asvetlov]

A teaser from requests documentation: http://docs.python-requests.org/en/master/user/advanced/#timeouts

They suggest that Most requests to external servers should have a timeout attached.

My opinion is:

1. We should keep existing client timeout strategy at least in aiohttp 3.X
2. connection-timeout + read-timeout is just an other feasible strategy. Let's assume you are requesting an other server for an answer. Doesn't matter how often it sends a portion of data you have a strong SLA for rejecting the response if it takes too long. This is the real case implemented by aiohttp right now.
3. We should support flexible timeouts. I believe after Client timeout refactoring #2768 landing we will satisfy all needs
4. We should document timeouts explicitly: if somebody if not satisfied by default provided ones this guy should get comprehensive information about alternative strategies.

P.S.
I don't want to get a breakage like aiobotocore and personally @thehesiod had.
I pretty sure that migration should be smooth.
I'm not convinced that defaults are so bad yet -- but I a strong request from alternative timeout settings, pretty well documented and very visible to newcomers.

[asvetlov]

I've changed the title because it is not an RFC violation but a request for new timeout mode.

[hubo1016]

The problem of current default is It is either too large or too small...

For many HTTP requests, the normal finish time is less than one second, so user would like a timeout in a few seconds; if it is not finished, the program regards the server as unavailable (might be: connection failed, server deadlock, server overloaded...)

Others will need a long timeout. Usually they trust the server they are calling, and believe that as long as the connection is working, there is no problem. If they have requests which take four minutes to be finished, they will have requests that could not be finished in five. The default tineout here will be surprising, because people think since It works on very long requests, It might not have a default timeout.

Clients are different from servers. They usually do not need a default timeout for security.

[asvetlov]

Fixed by #2972

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new