aiohttp icon indicating copy to clipboard operation
aiohttp copied to clipboard

Too many "async with"

Open flyinghu123 opened this issue 4 years ago • 21 comments

🐣 Is your feature request related to a problem? Please describe.

I'm always frustrated when I use get or post method. I must use "async with" to use get or post method. So there's a lot of "async with" nesting.

💡 Describe the solution you'd like

I hope that I can use it just like httpx. Ex:

async def main():
    async with httpx.AsyncClient() as session:
        resposne = await session.get("http://httpbin.org")  # I was able to release it without having to do it manually 
        print(resposne.text)

Describe alternatives you've considered

截图 Add "self.release" after "await self.read()". Because I don't need connection after this.

📋 Additional context

flyinghu123 avatar Jan 06 '21 14:01 flyinghu123

I think one problem with your proposed solution is that it would become less clear when you do need to close it, such as with:

async with resp:
    assert resp.status == 200

I think it could be reasonable to have a shortcut with an interface something like one of these:

await session.get(url, text=True)
await session.get_text(url)
await session.get_read(type="text")

With the return type either being the string returned from resp.text() or maybe a new ResponseRead type or similar, which just has a resp.text or resp.body attribute already prefilled with the response (instead of resp.text()).

Dreamsorcerer avatar Jan 06 '21 15:01 Dreamsorcerer

I'm not sure what exactly the issue is. The solution, which is already possible with aiohttp, e.g.:

async def main():
    async with aiohttp.ClientSession() as session:
        resposne = await session.get("http://httpbin.org")
    print(await resposne.text())

is what you're saying is an issue? since it involves using async with.

Regardless, you're not forced to use async with. You can always close the session afterwards yourself, e.g.:

async def main():
    session = aiohttp.ClientSession()
    resposne = await session.get("http://httpbin.org")
    print(await resposne.text())
    await session.close()

As for your alternative, as the documentation says:

It is not required to call release on the response object. When the client fully receives the payload, the underlying connection automatically returns back to pool. If the payload is not fully read, the connection is closed

The connection should already be released when the payload is fully read or when an error is encountered, so this would be redundant.

For code block usage, see https://docs.github.com/en/free-pro-team@latest/github/writing-on-github/creating-and-highlighting-code-blocks.

Harmon758 avatar Jan 06 '21 15:01 Harmon758

async def main():
    async with aiohttp.ClientSession() as session:
        resposne = await session.get("http://httpbin.org")
    print(await resposne.text())

That code looks wrong to me (the response is never closed explicitly). Compare https://docs.aiohttp.org/en/stable/client_quickstart.html#make-a-request

This issue is the inner async with:

async def main():
    async with aiohttp.ClientSession() as session:
        async with await session.get("http://httpbin.org") as response:
            print(await response.text())

Dreamsorcerer avatar Jan 06 '21 15:01 Dreamsorcerer

In fact I just ran your example, and it never printed or finished. I had to Ctrl+C.

Dreamsorcerer avatar Jan 06 '21 15:01 Dreamsorcerer

It's working fine here

flyinghu123 avatar Jan 06 '21 15:01 flyinghu123

@Harmon758 If there are no side effects, that's fine

flyinghu123 avatar Jan 06 '21 15:01 flyinghu123

async def main():
    async with aiohttp.ClientSession() as session:
        resposne = await session.get("http://httpbin.org")
    	print(await resposne.text())

That code looks wrong to me (the response is never closed explicitly). Compare docs.aiohttp.org/en/stable/client_quickstart.html#make-a-request

This issue is the inner async with:

async def main():
    async with aiohttp.ClientSession() as session:
        async with await session.get("http://httpbin.org") as response:
            print(await response.text())

@Dreamsorcerer Both of these will work

flyinghu123 avatar Jan 06 '21 15:01 flyinghu123

It will work if you indent the last line into the with block (which I see you changed now). But, then the response object is never explicitly closed, which is probably not what you want. So, you should then have a response.close() line as well.

This is the equivalent of doing open(...).read(), which will give you a warning if the dev flags are enabled in Python. As you should always explicitly close:

with open(...) as f:
    f.read()

Dreamsorcerer avatar Jan 06 '21 15:01 Dreamsorcerer

I don't think it can work if you're trying to read after the session is closed.

It will work if you indent the last line into the with block (which I see you changed now). But, then the response object is never explicitly closed, which is probably not what you want. So, you should then have a response.close() line as well.

This is the equivalent of doing open(...).read(), which will give you a warning if the dev flags are enabled in Python. As you should always explicitly close:

with open(...) as f:
    f.read()

@Dreamsorcerer I want to release it automatically after read it. I didn't have to read after that

flyinghu123 avatar Jan 06 '21 15:01 flyinghu123

And the shortcut available for that open() example is Path(...).read_text(). So, I'd say it would be reasonable to have a similar kind of shortcut.

Dreamsorcerer avatar Jan 06 '21 15:01 Dreamsorcerer

@flyinghu123 the response object does not hold the HTTP response body to give end-users more control. Imagine if the response is large, then it'd easy to consume gigabytes of memory. You'll get MemoryErrors all over the place without any possibility to process it in small chunks w/o polluting RAM. This is why there's a need for an async context manager.

webknjaz avatar Jan 06 '21 15:01 webknjaz

In fact I just ran your example, and it never printed or finished. I had to Ctrl+C.

Hmm, I think it might be the request to httpbin.org hanging. I did test it before and it was working, but now I'm encountering the same issue. Changing it to a different site, e.g. google.com, works though.

the response is never closed explicitly

Does the response need to be explicitly closed? I would have added it at the end, but I checked ClientResponse's __aexit__, and it seems to just release it, which should already be done after the payload is fully read.

Harmon758 avatar Jan 06 '21 15:01 Harmon758

@webknjaz If it's a big file, I can set "stream=True" just like "requests.get(url, stream=True)". Because most of the time it's small files. You can reduce the amount of code.

flyinghu123 avatar Jan 06 '21 15:01 flyinghu123

@webknjaz But, the text() etc. methods read it in full anyway, right? So, if these convenience methods are available, it seems reasonable to provide it even without the with.

e.g. txt = await session.get_text(url) Using such a convenience method (much like Path.read_text()) means that we are not interested in the Response object at all, and just want to get the content in full.

Dreamsorcerer avatar Jan 06 '21 15:01 Dreamsorcerer

@Dreamsorcerer I also think some shortcut methods aare needed.

flyinghu123 avatar Jan 06 '21 15:01 flyinghu123

the response is never closed explicitly

Does the response need to be explicitly closed? I would have added it at the end, but I checked ClientResponse's __aexit__, and it seems to just release it, which should already be done after the payload is fully read.

Ah, interesting. I just assumed it would close() it. So, technically, your example is safe to do then. But, it's not well documented, and I'd still err on the side of being explicit.

Dreamsorcerer avatar Jan 06 '21 15:01 Dreamsorcerer

In fact I just ran your example, and it never printed or finished. I had to Ctrl+C.

Hmm, I think it might be the request to httpbin.org hanging. I did test it before and it was working, but now I'm encountering the same issue. Changing it to a different site, e.g. google.com, works though.

the response is never closed explicitly

Does the response need to be explicitly closed? I would have added it at the end, but I checked ClientResponse's __aexit__, and it seems to just release it, which should already be done after the payload is fully read.

image

It seems so.

flyinghu123 avatar Jan 06 '21 16:01 flyinghu123

I agree that it's better to explicitly close, but in this case, I was trying to mimic the proposed solution in the issue as closely as possible. release does set _closed to True, which I think should make calling close afterwards redundant, but yeah, I don't think this is really guaranteed behavior anywhere in the documentation.

I unindented the last line because the indentation was unclear in the initial proposed solution and it seemed from the comment that it was meant to be outside the async with. It worked when I tested it initially, but it does seem to be inconsistently hanging, so you're probably right about there being issues with reading from the response after the session is closed. Although, closing the session explicitly, without using an async with, and then reading from the response afterwards seems to work consistently, so I'm not sure what the issue is exactly.

Harmon758 avatar Jan 06 '21 16:01 Harmon758

This issue reminds me about #4346. I think every lazy developer will be pleased if we finally implement it :)

greshilov avatar Jan 07 '21 11:01 greshilov

As one more example of shortcuts like this, sqlalchemy has:

result = await conn.execute(...)
thing = result.scalar()

And there is a shortcut for that: thing = conn.scalar(...)

Dreamsorcerer avatar Jan 07 '21 15:01 Dreamsorcerer

Just some additional comments on this. I believe that any time .read() (or associated methods) are called, this will close the connection, so technically the async with is not needed in these situations (I think this is documented somewhere). It could still be nice to have a shortcut method discussed above to remove this ambiguity though.

I would reject the very original suggestion though, to do something like httpx. httpx has the same async with, but under a separate API: https://www.python-httpx.org/async/#streaming-responses This is a particular problem, given that the response from httpx.get() and similar actually includes methods such as .aiter_text(), which gives the appearance of a streaming API, but has actually already loaded the entire body into memory, opening users to DoS attacks when they thought they were being careful. Because aiohttp is streaming by default, a user is only loading the full body into memory when it is obvious they are doing so (i.e. they are assigning the full response to a variable).

Dreamsorcerer avatar Nov 13 '23 22:11 Dreamsorcerer