aiohttp icon indicating copy to clipboard operation
aiohttp copied to clipboard

`await ClientResponse.json()` raises `TimeoutError` when `ClientSession` is closed

Open joeriddles opened this issue 3 years ago • 10 comments

🐞 Describe the bug await ClientResponse.json() raises asyncio.exceptions.TimeoutError when the ClientSession has already ended instead of raising an error indicating that reading the response body may be unsuccessful with an ended ClientSession. ClientResponse.json gets stuck in an endless loop, trying to read the response body that it cannot, until the TimeoutError is raised.

💡 To Reproduce Minimum code to reproduce (ran on Windows), may need to run several times to get it to hang and raise error.

from __future__ import annotations
import asyncio
import aiohttp

async def send() -> aiohttp.ClientResponse:
    timeout = aiohttp.ClientTimeout(total=30) # type: ignore
    async with aiohttp.ClientSession(timeout=timeout) as session:
        response = await session.get('https://jsonplaceholder.typicode.com/todos/')
    return response

async def main():
    response = await send()
    print('Received response...')
    data = await response.json()
    print(data)

if __name__ == "__main__":
    asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy())
    asyncio.run(main())

💡 Expected behavior ClientResponse should raise an error that the indicates the ClientSession has closed and cannot read the response body successfully.

📋 Logs/tracebacks

Received response...
Traceback (most recent call last):
  File "C:\Python38\lib\runpy.py", line 194, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "C:\Python38\lib\runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "C:\Users\user\Desktop\test_data\web_test\__main__.py", line 19, in <module>
    asyncio.run(main())
  File "C:\Python38\lib\asyncio\runners.py", line 43, in run
    return loop.run_until_complete(main)
  File "C:\Python38\lib\asyncio\base_events.py", line 616, in run_until_complete
    return future.result()
  File "C:\Users\user\Desktop\test_data\web_test\__main__.py", line 14, in main
    data = await response.json()
  File "C:\Users\user\Desktop\test_data\.venv\lib\site-packages\aiohttp\client_reqrep.py", line 1092, in json
    await self.read()
  File "C:\Users\user\Desktop\test_data\.venv\lib\site-packages\aiohttp\client_reqrep.py", line 1032, in read
    self._body = await self.content.read()
  File "C:\Users\user\Desktop\test_data\.venv\lib\site-packages\aiohttp\streams.py", line 370, in read
    block = await self.readany()
  File "C:\Users\user\Desktop\test_data\.venv\lib\site-packages\aiohttp\streams.py", line 392, in readany
    await self._wait("readany")
  File "C:\Users\user\Desktop\test_data\.venv\lib\site-packages\aiohttp\streams.py", line 306, in _wait
    await waiter
  File "C:\Users\user\Desktop\test_data\.venv\lib\site-packages\aiohttp\helpers.py", line 656, in __exit__
    raise asyncio.TimeoutError from None
asyncio.exceptions.TimeoutError

📋 Your version of the Python

$ python --version
Python 3.8.5

📋 Your version of the aiohttp/yarl/multidict distributions

$ python -m pip show aiohttp
Name: aiohttp
Version: 3.7.4.post0
Summary: Async http client/server framework (asyncio)
Home-page: https://github.com/aio-libs/aiohttp
Author: Nikolay Kim
Author-email: [email protected]
License: Apache 2
Location: c:\users\user\desktop\test_data\.venv\lib\site-packages  
Requires: chardet, typing-extensions, attrs, async-timeout, yarl, multidict
Required-by: 
$ python -m pip show multidict
Name: multidict
Version: 5.1.0
Summary: multidict implementation
Home-page: https://github.com/aio-libs/multidict
Author: Andrew Svetlov
Author-email: [email protected]
License: Apache 2
Location: c:\users\user\desktop\test_data\.venv\lib\site-packages
Requires: 
Required-by: yarl, aiohttp
$ python -m pip show yarl
Name: yarl
Version: 1.6.3
Summary: Yet another URL library
Home-page: https://github.com/aio-libs/yarl/
Author: Andrew Svetlov
Author-email: [email protected]
License: Apache 2
Location: c:\users\user\desktop\test_data\.venv\lib\site-packages
Requires: idna, multidict
Required-by: aiohttp

joeriddles avatar Apr 01 '21 23:04 joeriddles

I'm getting TimeoutError with ERROR:root:Error while closing connector: SSLError(1, '[SSL: APPLICATION_DATA_AFTER_CLOSE_NOTIFY] application data after close notify (_ssl.c:2745)')

And indeed if you read the body of the response before closing that is never happening.

I wonder if this related or I should create a new issue?

python -m pip show aiohttp
Name: aiohttp
Version: 4.0.0a1
Summary: Async http client/server framework (asyncio)
Home-page: https://github.com/aio-libs/aiohttp
Author: Nikolay Kim
Author-email: [email protected]
License: Apache 2
Location: /home/paulefou/other_projects/aiohttp
Requires: chardet, multidict, async-timeout, yarl, typing-extensions, frozenlist, aiosignal
Required-by: 

I've tested it on master branch

python --version Python 3.9.2

os: arch linux

ssl.OPENSSL_VERSION 'OpenSSL 1.1.1k 25 Mar 2021'

https://github.com/aio-libs/aiohttp/issues/3535

paulefoe avatar Apr 03 '21 03:04 paulefoe

Same here I think. I managed to reproduce with the code from @joeriddles once.

➜  tmp python3 -m pip show aiohttp
Name: aiohttp
Version: 3.7.4
Summary: Async http client/server framework (asyncio)
Home-page: https://github.com/aio-libs/aiohttp
Author: Nikolay Kim
Author-email: [email protected]
License: Apache 2
Location: /Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages
Requires: chardet, typing-extensions, async-timeout, yarl, attrs, multidict
Required-by: aiohttp-retry, pydnet

I run on OSX, python 3.8.8

andrea-cassioli-maersk avatar Aug 16 '21 06:08 andrea-cassioli-maersk

I've done some digging here as well and you can easily reproduce it by making sure you have a large enough response body so the StreamReader has not been fed the end of file yet before it gets closed by the exit of the context manager. It seems https://jsonplaceholder.typicode.com/comments consistently gives me a large enough body to trigger this.

m-vellinga avatar Mar 03 '22 15:03 m-vellinga

If you can write a test in a PR that reproduces the problem, that would likely help someone to work on a solution.

Dreamsorcerer avatar Mar 03 '22 18:03 Dreamsorcerer

Sounds like a good idea! I'll see if I can get around to doing that. One thing I'm still wondering though is what the right behaviour would be. I think @joeriddles suggestion is a good one so I might also add a commit to how to possibly do that.

m-vellinga avatar Mar 04 '22 10:03 m-vellinga

I stumbled over the same issue, I think it also conflicts with the actual documentation about client exception.

All exceptions are available as members of aiohttp module. [...] exception aiohttp.ClientError Base class for all client specific exceptions.

The raised TimeoutError however comes from asyncio.exceptions. Hence the following obviously doesn't cover it :-(

try:
    client.post(...)
except aiohttp.ClientError:
    # do something

MRuecklCC avatar Jun 23 '22 13:06 MRuecklCC

This caused us a few headaches trying to debug, can now reproduce consistently;

import aiohttp
import asyncio

async def main() -> None:
    timeout = aiohttp.ClientTimeout(total=10)
    async with aiohttp.ClientSession(timeout=timeout) as session:
        r = await session.get("https://stackoverflow.com/")

    txt = await r.text()
    print(txt)

if __name__ == "__main__":
    loop = asyncio.get_event_loop()
    loop.run_until_complete(main())

Currently this raises the timeout exception after the full timeout has elapsed. It would be preferable to raise an exception early if the context manager has exited and the body can no longer be consumed.

gryevns avatar Sep 27 '23 08:09 gryevns

That code doesn't look correct though. You're reading from a response object after the session is closed...

If you follow our examples, you should be doing something like:

    async with aiohttp.ClientSession(timeout=timeout) as session:
        async with session.get("https://stackoverflow.com/") as resp:
            txt = await resp.text()

Dreamsorcerer avatar Sep 27 '23 10:09 Dreamsorcerer

@Dreamsorcerer completely agree that the implementation is wrong in this instance.

As a developer it's quite an easy mistake especially when the code is not so simplified and across multiple files. It would be nice if aiohttp provided a warning/exception in such cases so it's obvious to the developer they're doing something wrong rather than waiting for the timeout to elapse.

In our case requests were working ~70% of the time, only large response content was failing which made this harder to debug, consider this example with a different site that does work:

import aiohttp
import asyncio


async def main() -> None:
    timeout = aiohttp.ClientTimeout(total=10)
    async with aiohttp.ClientSession(timeout=timeout) as session:
        r = await session.get("https://google.com/")

    txt = await r.text()
    print(txt)


if __name__ == "__main__":
    loop = asyncio.get_event_loop()
    loop.run_until_complete(main())

gryevns avatar Sep 27 '23 10:09 gryevns

@gryevns This burned me in exactly the same way. Had code split across two files where one file was making the request in the ClientSession, and the other file was awaiting the response. Worked most of the time, failed a few times a day.

Please don't allow us to await the response outside the ClientSession if it's going to randomly fail sometimes! Better to always be safe!

odie5533 avatar Feb 02 '24 03:02 odie5533