Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Too many "async with" #5385

Open
flyinghu123 opened this issue Jan 6, 2021 · 21 comments
Open

Too many "async with" #5385

flyinghu123 opened this issue Jan 6, 2021 · 21 comments

Comments

@flyinghu123
Copy link

flyinghu123 commented Jan 6, 2021

🐣 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

@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Jan 6, 2021

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()).

@Harmon758
Copy link
Member

Harmon758 commented Jan 6, 2021

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.

@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Jan 6, 2021

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
Copy link
Member

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

@flyinghu123
Copy link
Author

It's working fine here

@flyinghu123
Copy link
Author

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

@flyinghu123
Copy link
Author

flyinghu123 commented Jan 6, 2021

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

@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Jan 6, 2021

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()

@flyinghu123
Copy link
Author

flyinghu123 commented Jan 6, 2021

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

@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Jan 6, 2021

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.

@webknjaz
Copy link
Member

webknjaz commented Jan 6, 2021

@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.

@Harmon758
Copy link
Member

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.

@flyinghu123
Copy link
Author

@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.

@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Jan 6, 2021

@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.

@flyinghu123
Copy link
Author

@Dreamsorcerer
I also think some shortcut methods aare needed.

@Dreamsorcerer
Copy link
Member

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.

@flyinghu123
Copy link
Author

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.

@Harmon758
Copy link
Member

Harmon758 commented Jan 6, 2021

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.

@greshilov
Copy link
Contributor

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

@Dreamsorcerer
Copy link
Member

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
Copy link
Member

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants