Client timeouts

[asvetlov]

#2768

[codecov-io]

Codecov Report

Merging #2972 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2972      +/-   ##
==========================================
+ Coverage   98.09%   98.11%   +0.01%     
==========================================
  Files          42       42              
  Lines        7676     7698      +22     
  Branches     1338     1343       +5     
==========================================
+ Hits         7530     7553      +23     
  Misses         49       49              
+ Partials       97       96       -1

Impacted Files	Coverage Δ
aiohttp/client_proto.py	95.83% <100%> (+1.66%)	⬆️
aiohttp/client_reqrep.py	97.41% <100%> (-0.02%)	⬇️
aiohttp/connector.py	98.17% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d22c5ae...603017e. Read the comment docs.

[asvetlov]

Ready for review.
The implementation is done but I should write docs though.
The PR supports three timeouts only: total, connect and socket read.
We can extend the list easy but I doubt if we have to do it without real use cases.

[asvetlov]

@thehesiod you are the main feature requester.
Is the PR covers all aiobotocore needs?

[asvetlov]

Documentation is done

[thehesiod]

I think this still means that the connect timeout includes the time waiting for a connector from the pool no?

[asvetlov]

Nice catch!
Let's keep ClientTimeout.connect as is for backward compatibility but implement .sock_connect for actual connection establishment time.

[thehesiod]

Thanks for this!

we need only two timeouts:

1. connect timeout (waiting to connect to endpoint)
2. read timeout (waiting for data from endpoint)

also to completely get rid of our monkey patching:
3. ability to set socket timeout for a given response (comes from botocore requirement)

for 1) I think this doesn't fix it because it seems like it still includes the time for waiting for a connector from the pool. We achieve this here: https://github.com/aio-libs/aiobotocore/blob/master/aiobotocore/endpoint.py#L219 by wrapping a CeilTimeout around the _create_connection call. This is still heavy handed and instead should behave like requests: http://docs.python-requests.org/en/master/user/advanced/#timeouts, that is ideally this timeout would be eventually passed to socket.settimeout by the call loop.create_connection. It's rather unfortunate that loop.create_connection does not take a timeout parameter as you really want the underlying socket class to do this...as otherwise if you block your main loop you could trigger the timeout w/o the server actually trying to connect to the server.

for 2) we achieve this via wrapping the ResponseHandler._wait method as well as the .read method. It seems like a tighter coupling would be clearer than what's presented in this PR, no? Ideally around the time the request for data is made, and the time it is received. I think if there were a tighter coupling one wouldn't need as many changes.

for 3) we currently do this via: https://github.com/aio-libs/aiobotocore/blob/master/aiobotocore/endpoint.py#L137 by calling response._protocol.set_timeout. So it would be nice if there was a public method for this.

also could you explain the rationality for removing all the auto_decompress params in the description above? Would be going to understand and document for future reference.

[thehesiod]

btw, check this out: http://urllib3.readthedocs.io/en/latest/reference/urllib3.util.html#module-urllib3.util.timeout along with a pool_timeout: https://urllib3.readthedocs.io/en/1.4/pools.html#urllib3.connectionpool.HTTPConnectionPool.urlopen

[asvetlov]

ability to set socket timeout for a given response (comes from botocore requirement)

Do you need to set timeout after getting response object? Is passing a timeout to session.get(url, timeout=timeout) etc. satisfies your needs?

[thehesiod]

good news, so I dug a little into this and it seems 3) is no longer needed, it was only because requests didn't correctly support read timeouts (which they do now)

[asvetlov]

timeout would be eventually passed to socket.settimeout

socket.settimeout doesn't work for async sockets.

we currently do this by calling response._protocol.set_timeout.

Protocol has no set_timeout() method. Looks like you monkey-patching the protocol instance.

We can add something like set_timeout() method to the response object. It cannot work when the response is pre-fetched already but maybe it is not the case.

could you explain the rationality for removing all the auto_decompress params in the description above?

The parameter is a part of private API, now protocol.set_response_params() is called directly from client session, don't need to pass it to response through a request.

[asvetlov]

So my plan is: I'm going to implement pure connection timeout (without waiting from a pool).

Let's land then the PR and see what also do we need.

[asvetlov]

ClientTimeout.sock_accept is implemented and documented.

[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