Race condition in BaseConnector leads to exceeding connection limit #4936
Description
🐞 Describe the bug
It is possible for BaseConnector to exceed the limited number of connections. This is a result of a race condition/TOC-TOU bug on the available connections.
The bug arises from the following situation:
bc = BaseConnector(limit=1)
con1 = bc.connect(...) # makes first connection
con2 = bc.connect(...) # joins waiters queue
con1.release( ) # releases connection
con3 = bc.connect(...) # takes available connection
# con2 is resumed because of `set_result` call and creates second connection
...
con2.release()
con3.release()Note that the bug is only present if connection 3 is initiated before connection 2 resumes. The ordering of these two events is non-deterministic.
In Depth
When releasing a connection, _release_waiter will check that there is an available connection and call set_result to mark the future as done. This is assuming that this task will be resumed immediately but this isn't always the case: https://github.com/aio-libs/aiohttp/blob/3.7/aiohttp/connector.py#L588
If a coroutine is ran that calls connect, it will find there is an available connection and take it.
This will drop the key from BaseConnect._conns as it has taken the last connection: https://github.com/aio-libs/aiohttp/blob/3.7/aiohttp/connector.py#L569
If the future (triggered by set_result) now resumes, then proto = self._get(key) will be None, because the key was deleted above. Hence it will now create a new connection object.
https://github.com/aio-libs/aiohttp/blob/3.7/aiohttp/connector.py#L512
We have now created a second connection when limit was set to 1.
💡 To Reproduce
See pull request containing failing unit test.
💡 Expected behavior
Limit is observed appropriately
📋 Logs/tracebacks
📋 Your version of the Python
3.8.2
📋 Your version of the aiohttp/yarl/multidict distributions
Name: aiohttp
Version: 3.6.2
Name: multidict
Version: 4.7.6
Name: yarl
Version: 1.5.1