separate read + connect + request timeouts (#1523)

* attempt at separating read_timeout from conn_timeout

* fix coverage

- remove unused property set
- add test where TCPConnector does not have a conn_timeout

* fix to allow for unlimited timeouts

* fix coverage

* document changes

CHANGES.rst

@@ -4,6 +4,8 @@ CHANGES
 1.3.0 (XXXX-XX-XX)
 ------------------
 
+- separate read + connect + request timeouts # 1523
+
 - Fix polls demo run application #1487
 
 - Ignore unknown 1XX status codes in client #1353

aiohttp/client.py

@@ -27,6 +27,22 @@
 PY_35 = sys.version_info >= (3, 5)
 
 
+# 5 Minute default read and connect timeout
+DEFAULT_TIMEOUT = 5 * 60
+
+
+def _timeout_min(value1, value2):
+    # If neither value is None returns minimum of two, otherwise returns non-
+    # None value
+
+    assert value1 is not None
+
+    if value2 is None:
+        return value1
+
+    return min(value1, value2)
+
+
 class ClientSession:
     """First-class interface for making HTTP requests."""
 
@@ -39,7 +55,7 @@ def __init__(self, *, connector=None, loop=None, cookies=None,
                  response_class=ClientResponse,
                  ws_response_class=ClientWebSocketResponse,
                  version=aiohttp.HttpVersion11,
-                 cookie_jar=None):
+                 cookie_jar=None, read_timeout=None):
 
         if connector is None:
             connector = aiohttp.TCPConnector(loop=loop)
@@ -74,6 +90,7 @@ def __init__(self, *, connector=None, loop=None, cookies=None,
         self._connector = connector
         self._default_auth = auth
         self._version = version
+        self._read_timeout = read_timeout
 
         # Convert to list of tuples
         if headers:
@@ -124,7 +141,11 @@ def _request(self, method, url, *,
                  read_until_eof=True,
                  proxy=None,
                  proxy_auth=None,
-                 timeout=5*60):
+                 timeout=DEFAULT_TIMEOUT):
+
+        # NOTE: timeout clamps existing connect and read timeouts.  We cannot
+        # set the default to None because we need to detect if the user wants
+        # to use the existing timeouts by setting timeout to None.
 
         if version is not None:
             warnings.warn("HTTP version should be specified "
@@ -158,6 +179,17 @@ def _request(self, method, url, *,
         if proxy is not None:
             proxy = URL(proxy)
 
+        # optionally clamp timeouts to timeout parameter
+        read_timeout = self._read_timeout
+        conn_timeout = self._connector.conn_timeout
+        if timeout is not None:
+            read_timeout = _timeout_min(timeout, read_timeout)
+            conn_timeout = _timeout_min(timeout, conn_timeout)
+
+        # is this just the same as the connector's existing timeout?
+        if conn_timeout == self._connector.conn_timeout:
+            conn_timeout = None  # this will no-op the below timeout
+
         while True:
             url = URL(url).with_fragment(None)
 
@@ -170,9 +202,10 @@ def _request(self, method, url, *,
                 auth=auth, version=version, compress=compress, chunked=chunked,
                 expect100=expect100,
                 loop=self._loop, response_class=self._response_class,
-                proxy=proxy, proxy_auth=proxy_auth, timeout=timeout)
+                proxy=proxy, proxy_auth=proxy_auth, timeout=read_timeout)
 
-            with Timeout(timeout, loop=self._loop):
+            # None conn_timeout is a Timeout no-op
+            with Timeout(conn_timeout, loop=self._loop):
                 conn = yield from self._connector.connect(req)
             conn.writer.set_tcp_nodelay(True)
             try:

aiohttp/connector.py

@@ -175,6 +175,10 @@ def __enter__(self):
     def __exit__(self, *exc):
         self.close()
 
+    @property
+    def conn_timeout(self):
+        return self._conn_timeout
+
     @property
     def force_close(self):
         """Ultimately close connection on releasing if True."""

tests/test_client_functional.py

@@ -541,6 +541,46 @@ def handler(request):
         yield from client.get('/', timeout=0.01)
 
 
+@asyncio.coroutine
+def test_timeout_on_conn_reading_headers(loop, test_client):
+    # tests case where user did not set a connection timeout
+
+    @asyncio.coroutine
+    def handler(request):
+        resp = web.StreamResponse()
+        yield from asyncio.sleep(0.1, loop=loop)
+        yield from resp.prepare(request)
+        return resp
+
+    app = web.Application(loop=loop)
+    app.router.add_route('GET', '/', handler)
+
+    conn = aiohttp.TCPConnector(loop=loop)
+    client = yield from test_client(app, connector=conn)
+
+    with pytest.raises(asyncio.TimeoutError):
+        yield from client.get('/', timeout=0.01)
+
+
+@asyncio.coroutine
+def test_timeout_on_session_read_timeout(loop, test_client):
+    @asyncio.coroutine
+    def handler(request):
+        resp = web.StreamResponse()
+        yield from asyncio.sleep(0.1, loop=loop)
+        yield from resp.prepare(request)
+        return resp
+
+    app = web.Application(loop=loop)
+    app.router.add_route('GET', '/', handler)
+
+    conn = aiohttp.TCPConnector(loop=loop)
+    client = yield from test_client(app, connector=conn, read_timeout=0.01)
+
+    with pytest.raises(asyncio.TimeoutError):
+        yield from client.get('/', timeout=None)
+
+
 @asyncio.coroutine
 def test_timeout_on_reading_data(loop, test_client):