Fix a problem with connection waiters that are never awaited

[illia-v]

What do these changes do?

When a connector waits for multiple available connections with the same key, the described race condition can happen.

Let's say that we want to create 3 connections but there a no available ones due to a limit. Then:

1. The first future is added, BaseConnector.connect sets a deque 1 containing the future to a local waiters variable, the future is awaited, the context is switched.
2. The second future is added, BaseConnector.connect sets the deque 1 containing first and second futures to waiters, the second future is awaited, the context is switched.
3. One of the previous connections is closed, the first future is popped from the deque 1, its result is set, the context is switched.
4. Another previous connection is closed, the second future is popped from the deque 1 (the deque is empty), its result is set, the context is switched.
5. We returned to the context from the first step, the deque 1 is deleted from BaseConnector._waiters, ..., the context is switched.
6. The third future is added, BaseConnector.connect sets a new deque 2 containing the third future to waiters, the third future is awaited, the context is switched.
7. Asyncio returned to the context from the second step, it checks that the local waiters variable containing the deque 1 is empty, and deletes self._waiters[key]. But self._waiters[key] is the deque 2 at the moment. Therefore, the waiter represented as the third future will be never released. And the program will hang if no connect timeout is set.

Are there changes in behavior for the user?

No

Related issue number

Maybe #4258 and aio-libs/aiobotocore#738

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • The format is <Name> <Surname>.
  • Please keep alphabetical order, the file is sorted by names.
• Add a new news fragment into the CHANGES folder
  • name it <issue_id>.<type> for example (588.bugfix)
  • if you don't have an issue_id change it to the pr id after creating the pr
  • ensure type is one of the following:
    • .feature: Signifying a new feature.
    • .bugfix: Signifying a bug fix.
    • .doc: Signifying a documentation improvement.
    • .removal: Signifying a deprecation or removal of public API.
    • .misc: A ticket has been closed, but it is not of interest to users.
  • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

[codecov-io]

Codecov Report

Merging #4562 into master will increase coverage by 0.94%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4562      +/-   ##
==========================================
+ Coverage   96.59%   97.54%   +0.94%     
==========================================
  Files          43       43              
  Lines        8907     8907              
  Branches     1404     1404              
==========================================
+ Hits         8604     8688      +84     
+ Misses        175      100      -75     
+ Partials      128      119       -9

Impacted Files	Coverage Δ
aiohttp/connector.py	96.28% <100%> (+2.26%)	⬆️
aiohttp/worker.py	96.63% <0%> (+2.52%)	⬆️
aiohttp/helpers.py	96.61% <0%> (+2.9%)	⬆️
aiohttp/web_fileresponse.py	97.82% <0%> (+19.02%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea8f35e...79f370d. Read the comment docs.

[illia-v]

In [1]: from collections import defaultdict, deque                                                                                                                                                                                                                                                        

In [2]: dictionary = defaultdict(deque)                                                                                                                                                                                                                                                                   

In [3]: dq = dictionary['key']                                                                                                                                                                                                                                                                            

In [4]: dq.append('future')                                                                                                                                                                                                                                                                               

In [5]: dq is dictionary['key']                                                                                                                                                                                                                                                                           
Out[5]: True

In [6]: del dictionary['key']                                                                                                                                                                                                                                                                             

In [7]: dq                                                                                                                                                                                                                                                                                                
Out[7]: deque(['future'])

In [8]: dictionary                                                                                                                                                                                                                                                                                        
Out[8]: defaultdict(collections.deque, {})

In [9]: dictionary['key'].append('future')                                                                                                                                                                                                                                                                

In [10]: dictionary                                                                                                                                                                                                                                                                                       
Out[10]: defaultdict(collections.deque, {'key': deque(['future'])})

In [11]: dq is dictionary['key']                                                                                                                                                                                                                                                                          
Out[11]: False

[thehesiod]

this logic is getting impossible to reason with, how about something like this: thehesiod-forks#4 The issue I have right now is that _release_waiter is not async and doesn't appear to be easy to make async

[thehesiod]

actually I think I came up with a fix in my PR

[illia-v]

@thehesiod the race condition described in the first comment caused the aio-libs/aiobotocore#738 (comment) problem. When I was debugging it, it was pretty clear that a different deque with a same key was deleted from the dictionary. I checked twice, and therefore I don't agree that it is impossible :)

I'll take a look at your PR when I can

[illia-v]

@thehesiod I looked at your pull request and added two minor comments.

Af for me your solution looks better than the current implementation. Also, it seems to fix the problem mentioned in this PR too, I haven't checked it though.

If your changes are merged before the next release, I'm OK with closing this PR.
Otherwise, I'd like this one to be merged before the next release to fix the problem because it changes only one line and should not create any regressions

[thehesiod]

@illia-v I didn't open my PR against aiohttp, up to maintainers of aiohttp to see if they want that or not ( @asvetlov )

[pfreixes]

LGTM! @asvetlov would you mind giving your blessing for that fix?

[pfreixes]

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?

[illia-v]

It's a good idea. Thanks!

[codecov-commenter]

Codecov Report

Merging #4562 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4562      +/-   ##
==========================================
+ Coverage   97.58%   97.62%   +0.04%     
==========================================
  Files          43       43              
  Lines        8932     8929       -3     
  Branches     1406     1406              
==========================================
+ Hits         8716     8717       +1     
+ Misses         96       95       -1     
+ Partials      120      117       -3     

Impacted Files	Coverage Δ
aiohttp/connector.py	96.59% <100.00%> (+0.30%)	⬆️
aiohttp/web_fileresponse.py	97.82% <0.00%> (-0.55%)	⬇️
aiohttp/pytest_plugin.py	97.51% <0.00%> (+1.86%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d296d5...24238b5. Read the comment docs.

[codecov-io]

Codecov Report

Merging #4562 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4562      +/-   ##
==========================================
+ Coverage   97.63%   97.64%   +0.01%     
==========================================
  Files          43       43              
  Lines        8981     8978       -3     
  Branches     1411     1411              
==========================================
- Hits         8769     8767       -2     
  Misses         98       98              
+ Partials      114      113       -1     

Flag	Coverage Δ
#unit	97.64% <100.00%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiohttp/connector.py	96.80% <100.00%> (+0.14%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7d003e...15ce1ae. Read the comment docs.

[asvetlov]

LGTM, thanks!

[thehesiod]

@asvetlov welcome back :)

[github-actions]

💚 Backport successful

The PR was backported to the following branches:

• ✅ 3.7

[asvetlov]

Thanks, @thehesiod