ClientTimeout values of 0 don't disable timeouts

[rmoe]

🐞 Describe the bug
ClientTimeout values of 0 and None are treated differently. The docs here state:

All fields are floats, None or 0 disables a particular timeout check

and versions 3.6.3 and earlier do work this way. After #5091 this behavior was changed.

💡 To Reproduce
This simple example illustrates the issue:

timeout = aiohttp.ClientTimeout(
    total=90,
    connect=0,
    sock_connect=0,
    sock_read=0
)

async with aiohttp.ClientSession(timeout=timeout) as session:
    print(await session.get("https://google.com"))

With v3.6.3:

ryan@debian:~$ python -m pip show aiohttp
Name: aiohttp
Version: 3.6.3

ryan@debian:~$ python test.py
<ClientResponse(https://www.google.com/) [200 OK]>

With v3.7.4:

ryan@debian:~$ python -m pip show aiohttp
Name: aiohttp
Version: 3.7.4

ryan@debian:~$ python test.py

Traceback (most recent call last):
  File "/home/ryan/.virtualenvs/aio-test/lib/python3.9/site-packages/aiohttp/client.py", line 520, in _request
    conn = await self._connector.connect(
  File "/home/ryan/.virtualenvs/aio-test/lib/python3.9/site-packages/aiohttp/connector.py", line 535, in connect
    proto = await self._create_connection(req, traces, timeout)
  File "/home/ryan/.virtualenvs/aio-test/lib/python3.9/site-packages/aiohttp/connector.py", line 892, in _create_connection
    _, proto = await self._create_direct_connection(req, traces, timeout)
  File "/home/ryan/.virtualenvs/aio-test/lib/python3.9/site-packages/aiohttp/connector.py", line 999, in _create_direct_connection
    hosts = await asyncio.shield(host_resolved)
asyncio.exceptions.CancelledError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/ryan/.virtualenvs/aio-test/lib/python3.9/site-packages/aiohttp/client.py", line 520, in _request
    conn = await self._connector.connect(
  File "/home/ryan/.virtualenvs/aio-test/lib/python3.9/site-packages/async_timeout/__init__.py", line 45, in __exit__
    self._do_exit(exc_type)
  File "/home/ryan/.virtualenvs/aio-test/lib/python3.9/site-packages/async_timeout/__init__.py", line 92, in _do_exit
    raise asyncio.TimeoutError
asyncio.exceptions.TimeoutError

...

File "/home/ryan/.virtualenvs/aio-test/lib/python3.9/site-packages/aiohttp/client.py", line 524, in _request
    raise ServerTimeoutError(
aiohttp.client_exceptions.ServerTimeoutError: Connection timeout to host https://google.com

This timeout happens immediately.

With 3.7.4 if I change the timeout values from 0 to None it works correctly.

ryan@debian:~$ python -m pip show aiohttp
Name: aiohttp
Version: 3.7.4

ryan@debian:~$ python test.py
<ClientResponse(https://www.google.com/) [200 OK]>

💡 Expected behavior
Timeouts with values of 0 should be ignored (or the docs updated to say 0 doesn't disable the timeouts).

[Dreamsorcerer]

The change you reference doesn't appear to ever have been backported, so that change is not in aiohttp <4.

[rmoe]

I was trying different versions to narrow the issue down and saw it in the list of commits here v3.6.3...v3.7.0b0 and in the changelog . The behavior of the timeouts definitely changed between 3.6.3 and 3.7.0b0. When I removed the check here 3.7.x versions behaved the same as 3.6.3 in my test case.

[Dreamsorcerer]

Ah nevermind, I see now. The commit you mentioned is: 5996f85

But, the PR you linked to was for master, so shows different code. That was confusing me.

[Dreamsorcerer]

But, that check only stops it from calling ceil(). Surely, removing that would only make your code work by luck, in other words only if the connection is able to complete before the current second is up (i.e. ceil(current_time)).

There must be some other cause...

[Dreamsorcerer]

Or, maybe 3.6 only works by luck as well...

[rmoe]

But, that check only stops it from calling ceil(). Surely, removing that would only make your code work by luck, in other words only if the connection is able to complete before the current second is up (i.e. ceil(current_time)).

There must be some other cause...

I considered that and tried testing this earlier today. With the server below which waits 5 seconds before returning a request 3.6 works and 3.7 fails. My logic here is that if the rounding really meant that my 0s timeouts were actually something between 0 and 1 second instead of being disabled then a request which took 5 seconds should timeout after ~1s. With 3.6.3 I don't see that happening.

server.py

from aiohttp import web
import asyncio

routes = web.RouteTableDef()

@routes.get("/")
async def get(request):
    await asyncio.sleep(5)
    return web.json_response({"key":"value"})

if __name__ == "__main__":
    app = web.Application()
    app.add_routes(routes)
    web.run_app(app)

and using this client:

timeout = aiohttp.ClientTimeout(
    total=0,
    connect=0,
    sock_connect=0,
    sock_read=0
)

async with aiohttp.ClientSession(timeout=timeout) as session:
    print(await session.get("http://localhost:8080"))

With 3.6.3

ryan@debian:~$ time python test.py
<ClientResponse(http://localhost:8080) [200 OK]>
<CIMultiDictProxy('Content-Type': 'application/json; charset=utf-8', 'Content-Length': '16', 'Date': 'Tue, 09 Mar 2021 19:59:13 GMT', 'Server': 'Python/3.9 aiohttp/3.6.3')>


real    0m5.183s
user    0m0.160s
sys     0m0.021s

and if I change any of the timeouts to a non-zero value it does what I expect. With sock_read=2:

ryan@debian:~$ time python test.py
Traceback (most recent call last):
...
aiohttp.client_exceptions.ServerTimeoutError: Timeout on reading data from socket

real    0m2.183s
user    0m0.162s
sys     0m0.020s

[Dreamsorcerer]

Thanks, so if I try with that server, then I see it fails with the unmodified 3.7 code. However, if I change the line to add 0.1 seconds:
now = loop.time() + 0.1

Then it works fine. So, it seems like the timeout is not being respected, as long as it has opened the connection before the timeout, or something along those lines...

[Dreamsorcerer]

OK, I think I've found the issue. The sock_connect will always do this timeout. So, it was working by luck as we initially thought in the old code. But, it is only opening the connection that is affected, which is why your test server doesn't reproduce it.

[Dreamsorcerer]

Appears to already have been fixed on master anyway. Copying the change into 3.8.