-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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 DNSCache race-condition #2621
Conversation
fe80327
to
7d4dcf4
Compare
7d4dcf4
to
063993a
Compare
Codecov Report
@@ Coverage Diff @@
## master #2621 +/- ##
==========================================
+ Coverage 97.92% 97.92% +<.01%
==========================================
Files 38 38
Lines 7265 7268 +3
Branches 1259 1259
==========================================
+ Hits 7114 7117 +3
Misses 47 47
Partials 104 104
Continue to review full report at Codecov.
|
aiohttp/connector.py
Outdated
return islice(self._addrs_rr[host], len(self._addrs[host])) | ||
addrs = self._addrs[host].copy() | ||
shuffle(addrs) | ||
return addrs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not keep the original round robin but making sure that we get rid of the corner case ? such as
>>> addrs = cycle([1, 2, 3, 4])
>>> list(islice(addrs, 4))
[1, 2, 3, 4]
>>> next(addrs)
1
>>> list(islice(addrs, 4))
[2, 3, 4, 1]
Following your idea of using an ad-hoc list per call to next_addrs
it's just a matter of make a next after built the list to make sure that we always use a shifted list per call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because, anyway, cycle will start from actually random element. Why not to randomize explicitly ? Why we should stick on round-robin ?
In [7]: timeit list(islice(xxx, 4))
The slowest run took 15.44 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 447 ns per loop
In [8]: timeit shuffle(qwe)
The slowest run took 5.42 times longer than the fastest. This could mean that an intermediate result is being cached.
100000 loops, best of 3: 2.82 µs per loop
Well, it seems you are right....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I fixed.
fca0258
to
668fc3c
Compare
So, ready to be merged? flake8 is failing for some reason. Up to yo to merge it once it is fixed. |
Let me review soon. |
Looks good |
668fc3c
to
8dd457f
Compare
aa7824d
to
2f277f9
Compare
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. |
Are there changes in behavior for the user?
Related issue number
#2620
Checklist
CONTRIBUTORS.txt
CHANGES
folder<issue_id>.<type>
for example (588.bugfix)issue_id
change it to the pr id after creating the pr.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.