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

ClientSession created in aiohttp.request() remains unclosed #2036

Closed
das7pad opened this issue Jun 28, 2017 · 13 comments
Closed

ClientSession created in aiohttp.request() remains unclosed #2036

das7pad opened this issue Jun 28, 2017 · 13 comments
Labels

Comments

@das7pad
Copy link
Contributor

das7pad commented Jun 28, 2017

Long story short

Using aiohttp.request() to file http requests spamms the logfiles.

The aiohttp.ClientSession created in aiohttp.request() should be closed gracefully, but currently the session reference is just deleted.

Expected behaviour

No resource warning from using aiohttp.request

Actual behaviour

Unclosed aiohttp.ClientSessions messages for every request
see tdryer/hangups#340

Steps to reproduce

call aiohttp.request
https://github.com/tdryer/hangups/blob/master/hangups/http_utils.py#L33-L36

From another project you can find a tester with results on different aiohttp version here:
https://gist.github.com/das7pad/e8f63175377576a0cb3466d20a417fa5

Your environment

python v3.5.3

aiohttp v2.2.0

@das7pad
Copy link
Contributor Author

das7pad commented Jun 28, 2017

I already have a fix for this issue but as version 2.2.0 is not added to the v2 release branch, I can not submit a pull request to it that could pass all the checks then.
Using the main branch currently throws AssertionErrors for the refactored header handling in version 2.3.x and bad lower/upper case usage.

I created a branch from the version tag 2.2.0 and commited there: https://github.com/das7pad/aiohttp/tree/close-created-session
Feel free to cherrypick the commit das7pad@6052619 instead of a PR or guide me to which branch I should commit it to.

@asvetlov
Copy link
Member

I recommend switch from aiohttp.request() to ClientSession.request() with proper session closing.

@das7pad
Copy link
Contributor Author

das7pad commented Jun 29, 2017

But this change literally deprecates aiohttp.request().
Application X is now forced to have a more complex setup for a single http request with creating a session, then filing the request from this session and closing the request and session at the end. In addition it has to extend the Exception-handling to close the additional session resource.

In python version 3.5 one could simply use async with for the session and request, but for version 3.4 it is more than that:
https://gist.github.com/das7pad/4ff20e6a8e7e3242bffb9ee9c30809d3
Every application has to change the http requests now when using aiohttp.request before.

@fafhrd91
Copy link
Member

we can silence warning for aiohttp.request()

@asvetlov
Copy link
Member

we can silence warning for aiohttp.request()

Good to me.

Honestly I want to remove aiohttp.request() at all. Maybe along with future drop of Python 3.4 support.
Is there reason to keep the function?

@fafhrd91
Copy link
Member

simplicity.
@asvetlov I noticed, you like ceremony a lot :)

@asvetlov
Copy link
Member

aiohttp is OSS project.
Our changes and overall design should be clean and visible for library users.
It's not a ceremony for ceremony but a way to reduce breaking changes.

Well, I could buy your argument for keeping aiohttp.request(). Forcing usage to async with aiohttp.request() could get a place for proper session shutdown.

@fafhrd91
Copy link
Member

What could be cleaner than aiohttp.request() ?
Even with async with you can get same warning, even with with ClientSession you can get warning. Python just can't enforce right usage. Did you forget your change to clientresponse? Closing session after exit from context is not safe operation.

@asvetlov
Copy link
Member

Ok, let's just suppress a warning

@das7pad
Copy link
Contributor Author

das7pad commented Jun 29, 2017

Closing session after exit from context is not safe operation.

@fafhrd91 is that a note to this line? https://gist.github.com/das7pad/4ff20e6a8e7e3242bffb9ee9c30809d3#file-aiohttp_fetch_comparison-py-L29

@fafhrd91
Copy link
Member

@das7pad I am talking about using session as async context manager. you example is fine.

@asvetlov
Copy link
Member

asvetlov commented Feb 9, 2018

The functionality was changed and fixed in aiohttp 3.0

@lock
Copy link

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants