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

Fix a problem with connection waiters that are never awaited #4562

Merged
merged 5 commits into from
Oct 17, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES/4562.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a problem with connection waiters that are never awaited.
1 change: 1 addition & 0 deletions CONTRIBUTORS.txt
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ Igor Alexandrov
Igor Davydenko
Igor Mozharovsky
Igor Pavlov
Illia Volochii
Ilya Chichak
Ingmar Steen
Ivan Larin
Expand Down
5 changes: 4 additions & 1 deletion aiohttp/connector.py
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,10 @@ async def connect(self, req: 'ClientRequest',

raise e
finally:
if not waiters:
# `waiters` may be deleted from `self._waiters` by another
# coroutine, and `self._waiters[key]` may contain another
# deque, the later should not be deleted.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that the issue is that we are caching the self._waiters[key] at the local level by using the local variable waiters what about simply doing a check with using the none cached version, like:

if key in self._waiters and not self._waiters[key]:
    ...

Maybe it's more clear.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good idea. Thanks!

if not waiters and self._waiters[key] is waiters:
try:
del self._waiters[key]
except KeyError:
Expand Down
39 changes: 39 additions & 0 deletions tests/test_connector.py
Original file line number Diff line number Diff line change
Expand Up @@ -2290,3 +2290,42 @@ async def send_dns_cache_hit(self, *args, **kwargs):
connector._throttle_dns_events[key] = EventResultOrError(loop)
traces = [DummyTracer()]
assert await connector._resolve_host("", 0, traces) == [token]


async def test_connector_does_not_remove_needed_waiters(loop, key) -> None:
proto = create_mocked_conn(loop)
proto.is_connected.return_value = True

req = ClientRequest('GET', URL('https://localhost:80'), loop=loop)
connection_key = req.connection_key

connector = aiohttp.BaseConnector()
connector._available_connections = mock.Mock(return_value=0)
connector._conns[key] = [(proto, loop.time())]
connector._create_connection = create_mocked_conn(loop)
connector._create_connection.return_value = loop.create_future()
connector._create_connection.return_value.set_result(proto)

dummy_waiter = loop.create_future()

async def await_connection_and_check_waiters():
connection = await connector.connect(req, [], ClientTimeout())
try:
assert connection_key in connector._waiters
assert dummy_waiter in connector._waiters[connection_key]
finally:
connection.close()

async def allow_connection_and_add_dummy_waiter():
# `asyncio.gather` may execute coroutines not in order.
# Skip one event loop run cycle in such a case.
if connection_key not in connector._waiters:
await asyncio.sleep(0)
connector._waiters[connection_key].popleft().set_result(None)
del connector._waiters[connection_key]
connector._waiters[connection_key].append(dummy_waiter)

await asyncio.gather(
await_connection_and_check_waiters(),
allow_connection_and_add_dummy_waiter(),
)