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

Always close implicit connector, even for keepalive responses. #79

Merged
merged 2 commits into from Jun 22, 2014

Conversation

asvetlov
Copy link
Member

If network resource returns keepalive response an implicit connection is not closed now.
That's leads to resource leaks and eventually to OSError: [Errno 24] Too many open files.
I guess to always close implicit connection and process keepalives only if explicit connector has been passed to aiohttp.request.

See http://stackoverflow.com/questions/24298095/connections-arent-closing-with-python3-asyncio-concurrent-http-get-requests/24312441?noredirect=1 as initial report for the issue

@fafhrd91
Copy link
Member

I think 'force_close' should be TCPConnector parameter. should we also introduce keep-alive timer for each opened connection, similar to what server handler does?

@asvetlov
Copy link
Member Author

force_close for TCPConnector only or for BaseConnector also?
I like the second.

About keep-alive timer -- nice to have, but timer can be implemented in another issue.

@fafhrd91
Copy link
Member

Agree on both

@asvetlov
Copy link
Member Author

New patch is ready.
@fafhrd91 please review.

@fafhrd91
Copy link
Member

Lgtm

asvetlov added a commit that referenced this pull request Jun 22, 2014
Always close implicit connector, even for keepalive responses.
@asvetlov asvetlov merged commit e360b1b into master Jun 22, 2014
@asvetlov asvetlov deleted the close_implicit_connector branch June 22, 2014 15:01
@asvetlov
Copy link
Member Author

Thanks

@asvetlov
Copy link
Member Author

I think the fix worth to be published in 0.8.2 ASAP.
BTW I've got the same in my production code but did not figure out the source of problem.
After looking on stack overflow issue we've changed own code to explicit connector usage last Friday, but maybe some user will catch the same error.

@fafhrd91
Copy link
Member

ok, i'll make release later today.

@asvetlov
Copy link
Member Author

Thank you again.

@georgicodes
Copy link

@asvetlov and @fafhrd91. Thanks for fixing this! I was wondering what was going on.

@lock
Copy link

lock bot commented Oct 30, 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.

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

Successfully merging this pull request may close these issues.

None yet

3 participants