Can't read response content after checking response.ok

[adamko147]

🐞 Describe the bug
For failed requests, ClientResponse is "released" after calling to resposne.ok and therefore can't read response.text() after checking response.ok

💡 To Reproduce

async with session.get("url-returning-400+") as response:
    if not response.ok:
       msg = await response.text() # raises ClientConnectionError('Connection closed')

does not work, while

async with session.get("url-returning-400+") as response:
    if response.status != 200:
        msg = await response.text() # works just fine

works just fine

💡 Expected behavior
I'm not sure this is expected behaviour. response.ok is read-only property and I believe it should not change the state of the response object. Also, failed requests may contain response body, e.g. with error description, so I'm expecting await response.text() to work properly after checking response.ok

📋 Your version of the Python

$ python --version
Python 3.8.6

📋 Your version of the aiohttp/yarl/multidict distributions

$ python -m pip show aiohttp
Name: aiohttp
Version: 3.7.3
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: /Users/I028910/sbn/job-context-store/.venv/lib/python3.8/site-packages
Requires: async-timeout, attrs, typing-extensions, chardet, yarl, multidict
Required-by: sbn-aio-utils, aioresponses, aiohttp-retry

$ 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: /Users/I028910/sbn/job-context-store/.venv/lib/python3.8/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: /Users/I028910/sbn/job-context-store/.venv/lib/python3.8/site-packages
Requires: idna, multidict
Required-by: aiohttp

[webknjaz]

await msg = response.text()

This syntax is invalid. Instead, it should be:

msg = await response.text()

[Dreamsorcerer]

Looking at the implementation, I think this is expected behaviour.

Typical usage is:

if resp.ok:
    msg = await resp.text()

The expectation appears to be that you would not want to read anything from the connection if resp.ok is False. i.e. It releases when the resp is an error, not when it's successful.

[Dreamsorcerer]

Hmm, appears to be a fix for this issue: #3364

So, maybe it shouldn't apply to resp.ok?

[webknjaz]

res.ok calls resp.raise_for_status which in turn calls resp.release() in case of errors, marking the connection as wasted: https://github.com/aio-libs/aiohttp/blame/025d940/aiohttp/client_reqrep.py#L936

I think this is indeed a bug. The best way to fix it is to put the 400 <= self.status check into the ok property and then, we could use self.ok in the check that raise_for_status() has. So it'd work vice versa.

The expectation appears to be that you would not want to read anything from the connection if resp.ok is False.

I think it's not an expectation but something that's been overlooked during the review of #4711.

cc @EvanKepner

[adamko147]

await msg = response.text()

This syntax is invalid. Instead, it should be:

msg = await response.text()

Thanks, updated, that was just "stupid copy/paste" :)