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

BaseConnector limit and limit_per_host don't work with tracing enabled #5259

Closed
bobh66 opened this issue Nov 19, 2020 · 4 comments · Fixed by #5285
Closed

BaseConnector limit and limit_per_host don't work with tracing enabled #5259

bobh66 opened this issue Nov 19, 2020 · 4 comments · Fixed by #5285
Labels

Comments

@bobh66
Copy link
Contributor

bobh66 commented Nov 19, 2020

🐞 Describe the bug
The BaseConnector limit and limit_per_host attributes don't function properly when tracing is enabled and the connection is being reused. BaseConnector.connect() calls _available_connections(), and when it gets back a number > 0, it will await any configured traces before acquiring the connection. That is enough time for another thread to jump in and acquire the same connection, allowing more than the configured number of connections to exist simultaneously. connect() needs to acquire the connection, possibly with a placeholder, before awaiting the traces.

💡 To Reproduce

  1. Configure a limit and/or limit_per_host on a TCPConnection
  2. Configure tracing for on_connection_reuseconn()
  3. Queue up a large number of requests that exceed the limits specified
  4. Observe that more than the configured limit number of queries are run simultaneously

💡 Expected behavior
There should not be more than the configured limit number of connections active at any given time

📋 Logs/tracebacks

📋 Your version of the Python

$ python --version
Python 3.7.3
...

📋 Your version of the aiohttp/yarl/multidict distributions

$ python -m pip show aiohttp
Name: aiohttp
Version: 3.6.2
Summary: Async http client/server framework (asyncio)
Home-page: https://github.com/aio-libs/aiohttp
Author: Nikolay Kim
Author-email: [email protected]
License: Apache 2
Location: /usr/local/lib/python3.7/site-packages
Requires: chardet, yarl, attrs, async-timeout, multidict
Required-by: spe, faust, bravado-asyncio, aiohttp-cors
$ python -m pip show multidict
Name: multidict
Version: 4.7.5
Summary: multidict implementation
Home-page: https://github.com/aio-libs/multidict
Author: Andrew Svetlov
Author-email: [email protected]
License: Apache 2
Location: /usr/local/lib/python3.7/site-packages
Requires: 
Required-by: yarl, aiohttp
$ python -m pip show yarl

Name: yarl
Version: 1.4.2
Summary: Yet another URL library
Home-page: https://github.com/aio-libs/yarl/
Author: Andrew Svetlov
Author-email: [email protected]
License: Apache 2
Location: /usr/local/lib/python3.7/site-packages
Requires: idna, multidict
Required-by: faust, aiohttp

📋 Additional context

client

@bobh66 bobh66 added the bug label Nov 19, 2020
@asvetlov
Copy link
Member

Good catch!
Would you make a PR?
Writing tests for such things is hard; mock-based tests are not reliable.
I'm ok to review the patch even without the test suite updated (at least start the reviewing, maybe we figure out how to add a good test after the main patch appears).

@bobh66
Copy link
Contributor Author

bobh66 commented Nov 23, 2020

Thanks @asvetlov - I agree on the unit test - I'll submit a PR without a test for now and if someone has a way to test it I'll be glad to add it in.

@bobh66
Copy link
Contributor Author

bobh66 commented Nov 28, 2020

Thanks!

@asvetlov
Copy link
Member

Welcome!

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

Successfully merging a pull request may close this issue.

2 participants