Release HTTP response before raising status exception

[Grendel7]

What do these changes do?

A bugfix for issue #3364 - it releases the HTTP response before the ClientResponse exception is thrown.

Are there changes in behavior for the user?

If there is some way to get the HTTP response from a ClientResponseException (which, AFAIK, isn't possible), the response would be closed already.

Beyond that, no behavioral changes except for that connections are closed properly.

Related issue number

#3364

Checklist

• I think the code is well written
• Unit tests for the changes exist: I could not reproduce this issue with aiohttp's server.
• Documentation reflects the changes: I could not find relevant docs.
• If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • The format is <Name> <Surname>.
  • Please keep alphabetical order, the file is sorted by names.
• Add a new news fragment into the CHANGES folder
  • name it <issue_id>.<type> for example (588.bugfix)
  • if you don't have an issue_id change it to the pr id after creating the pr
  • ensure type is one of the following:
    • .feature: Signifying a new feature.
    • .bugfix: Signifying a bug fix.
    • .doc: Signifying a documentation improvement.
    • .removal: Signifying a deprecation or removal of public API.
    • .misc: A ticket has been closed, but it is not of interest to users.
  • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

[webknjaz]

I could not reproduce this issue with aiohttp's server.

We need regression tests. And if you cannot reproduce an issue within aiohttp, is it a real bug then? I'd like more details on this before proceeding. Maybe togather we could come up with a solution...

[codecov-io]

Codecov Report

Merging #3365 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3365   +/-   ##
=======================================
  Coverage   97.96%   97.96%           
=======================================
  Files          44       44           
  Lines        8413     8413           
  Branches     1374     1374           
=======================================
  Hits         8242     8242           
  Misses         72       72           
  Partials       99       99

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c13f8c6...431ddd0. Read the comment docs.

[Grendel7]

Thank you @webknjaz for checking in.

I can confirm this issue is real. However, the only times I have been able to confirm this issue with a remote host and a large response body.

Come to think, it might not actually be a limitation of aiohttp's server, but a problem with local networking. I haven't tried to run an aiohttp server on a remote system yet to see if I could reproduce the issue.

It might be that local networking is the issue. And by issue, I mean I'm not sure whether either

• the issue does not occur on a loopback interface; or
• the command I used to check it doesn't work on loopback interfaces.

Question: Does Python or one of the lower level components of aiohttp expose information about TCP connection statuses? If so, that would make testing the connection states a lot easier.

This was a script I used to test a big server response:

server.py

from aiohttp import web, ClientSession


async def body(request):
    client = ClientSession()
    async with client.get('https://en.wikipedia.org/wiki/Main_Page#/media/File:Lewis_Carroll_-_Henry_Holiday_-_Hunting_of_the_Snark_-_Plate_9.jpg') as response:
        body = await response.read()
    await client.close()
    return web.Response(body=str(body)+str(body)+str(body), status=404)

app = web.Application()
app.add_routes([web.get('/', body)])

web.run_app(app)

[asvetlov]

Better fix is calling self.close() from resp.raise_for_status().
Unit-test for resp.closed looks easy.
@HansAdema would you improve the PR?

[Grendel7]

No problem, I've updated the PR with a check on response.closed in the tests.

[asvetlov]

Thanks @HansAdema !

[haizaar]

Hi guys,

Sorry for necroing, but after quite some time we've upgraded to recent version of aiohttp and lots of our error handling code stopped working due to the change introduced in this PR. Here example of the code we use for error handling:

    async with aiohttp.ClientSession() as s:
        res = await s.get("https://google.com/not-such-thing")
        try:
            res.raise_for_status()
        except aiohttp.ClientResponseError as err:
            print("Failed. The error body:")
            print(await res.text())

Awaiting for res.text() now raises ClientConnectionError: Connection closed.

If I understand right, the reason for the fix was to prevent connection leaking if exception was raised, right?

Anyhow, for us it is quite a big breaking change. What would be your recommendation? Switch to manual status code parsing? Namely:

    async with aiohttp.ClientSession() as s:
        res = await s.get("https://google.com/not-such-thing")
        if 400 <= res.status:
            print("Failed. The error body:")
            print(await res.text())

Thank you.

/CC @worroc @motinani

[elupus]

Ran into this as well. In general it's annoying that the ClientResponseError doesn't include body. But now the simple workaround doesn't exist. You have to implement your own raise_for_status method.

[asvetlov]

I'm sorry, guys, for the design mistake made years ago.

If you need a response body the manual status code parsing is maybe the best option available (currently raise_for_status() contains 12 code lines).

Providing a response body as an attribute of ClientResponseError is not an option because the response has potentially unlimited size; it leads to a high memory footprint (not a leak technically but the very not desirable behavior).

[elupus]

I don't understand the need for the close in method thou. Either there is a context manager or you need be prepared for manual closing in some finally clause right. I suppose it would be an easy case to miss though.

Work around isn't too hard though.

[asvetlov]

Generally speaking, you don't know at the exception catch point if the response was closed or not.
It generates an additional point of confusing.

[haizaar]

I'll second asvelov here. Though this change caught us off guard and we had to fix thing on the spot, eventually I think it brings clarity wrt resource management.