Client raises asyncio.TimeoutError

[poofeg]

Describe the bug

Client raises asyncio.TimeoutError, although according to the documentation it seems that aiohttp.ServerTimeoutError should be raised.

To Reproduce

1. Run nc -l 8000
2. Run:

import aiohttp
import asyncio


async def main():
    timeout = aiohttp.ClientTimeout(total=10)
    async with aiohttp.ClientSession(timeout=timeout) as session:
        try:
            async with session.get('http://127.0.0.1:8000'):
                pass
        except aiohttp.ServerTimeoutError:
            print('aiohttp.ServerTimeoutError')
        except aiohttp.ClientError:
            print('aiohttp.ClientError')
        except asyncio.TimeoutError:
            print('asyncio.TimeoutError')


if __name__ == '__main__':
    asyncio.run(main())

3. Wait 10 seconds
4. Get result: "asyncio.TimeoutError"

Expected behavior

Result: "aiohttp.ServerTimeoutError"

Logs/tracebacks

asyncio.TimeoutError

Python Version

3.11.0

aiohttp Version

3.8.3

multidict Version

6.0.2

yarl Version

1.8.1

OS

macOS 12.6.1

Related component

Client

Additional context

No response

Code of Conduct

• I agree to follow the aio-libs Code of Conduct

[webknjaz]

Hey @graingert, can this be related to your patch #6877?

[webknjaz]

Looks like the code does raise ServerTimeoutError on (1) connection timeout
(

aiohttp/aiohttp/client_proto.py

Line 197 in 0d39c84

exc = ServerTimeoutError("Timeout on reading data from socket")

) and on read timeout (

aiohttp/aiohttp/client.py

Line 504 in 0d39c84

raise ServerTimeoutError(

)

[poofeg]

This is not a new behavior. At least since version 2.3.10, this case raises asyncio.TimeoutError exception.

[nick-merrill]

Thanks for posting this question @poofeg. I had thought we had handled timeouts in our retry logic, but we didn't identify this exception as one that needs to be handled as well.

I would suggest:

1. Update the documentation to clarify that asyncio.TimeoutError may be raised in the event of a timed-out request.
2. Consider (but probably avoid) raising ServerTimeoutError instead of asyncio.TimeoutError in this case.

Do people agree the documentation should be updated to call out this behavior?

[Dreamsorcerer]

@webknjaz @bdraco Any opinion on these timeouts?
Currently (atleast on master), ConnectionTimeoutError is thrown for connect and sock_connect, SocketTimeoutError for sock_read, and TimeoutError for total.
As it's a total timeout, not specific to the server connection, maybe it makes sense to keep as a TimeoutError?

Also, I noticed that ClientResponse.start() reenters the same timer context that is already being used in the parent Client._request() call, which seems redundant.

but we didn't identify this exception as one that needs to be handled as well.

Not as well. It's the only one that needs to be handled as it is the parent for all timeout exceptions.

[Dreamsorcerer]

Also, I noticed that ClientResponse.start() reenters the same timer context that is already being used in the parent Client._request() call, which seems redundant.

This could have been planned for future timeouts though (there are several proposed timeouts commented out in ClientTimeout).

[bdraco]

I don't have a strong opinion either way as long as the docs reflect what actually happens. I would consider this issue closed by the changes proposed in #8968