Throttle simultaneous DNS requests #1924 #2111

pfreixes · 2017-07-18T20:47:48Z

What do these changes do?

Allows to the connector throttle simultaneous DNS requests for a specific
domain when the DNS cache is enabled, having this enabled by default.

This feature get rid of the dogpile side effect when there is a miss in the cache, having
just one request doing the DNS resolution - at hostname granularity, requests that continue arriving while the ongoing DNS resolution is not finished will wait till it finishes.

When the DNS cache is not used. All requests will end up making a DNS query.

Are there changes in behavior for the user?

No

Related issue number

No

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 entry to CHANGES.rst
- Choose any open position to avoid merge conflicts with other PRs.
- Add a link to the issue you are fixing (if any) using #issue_number format at the end of changelog message. Use Pull Request number if there are no issues for PR or PR covers the issue only partially.

Added a new option at TCPConnector level called `throttle_dns` that allows to the connector throttle simulatnewous DNS requests for a specific domain. This functionality its only compatible when the DNS cache is enabled and it is dissabled by default. Therefore, many requests can end up making the same DNS resolution.

…ests

Will help the user to throttle DNS requests to the same domain. By design it will happen always when many URLs behind the same domain are scheduled at the same time, perhaps via *gather* The Event class is no longer a derivated class of the asyncio.locks.Event, and makes a full copy of this one to avoid future issues.

pfreixes · 2017-07-18T21:46:11Z

Trying to get a green pipeline, just a copy of the #1924. Now issues with the output of the coverage having a really weird error

INTERNALERROR>   File "/home/travis/virtualenv/python3.7-dev/lib/python3.7/site-packages/_pytest/main.py", line 105, in wrap_session
INTERNALERROR>  ........

Again

it works on my computer

Any recommendation to fix that issue with the pipeline? @asvetlov @kxepal @fafhrd91

fafhrd91 · 2017-07-18T21:50:03Z

do not worry about "Python: nightly".
your build actually fail on "migration.rst:152:api’s:"

https://travis-ci.org/aio-libs/aiohttp/jobs/255018892#L491

kxepal · 2017-07-19T05:36:21Z

aiohttp/locks.py

+from .helpers import create_future
+
+
+class Event:


Isn't it possible to inherint from asyncio.Event and override set method instead? I'm not sure we want to maintain whole own Even lock.

No longer used that pattern

pfreixes · 2017-07-19T20:41:17Z

Thanks for the tip @fafhrd91 fixed.

Aside of that, after some research I decided to get rid of the full Event class, the same behavior can be reached with the new and simple wrapper called ErrorfulOneShotEvent (thanks to Nathaniel for that approx [1])

Disclaimer, there is still a red-flag raised by @asvetlov regarding the Futures related to these tasks pending to be awakened, the whole thread can be followed here [2]. BTW my opinion about that is still the same, the proposed implementation is not nocive at all or at least is guided by the same principles than the current one.

[1] https://mail.python.org/pipermail/python-ideas/2017-July/046486.html
[2] #1924

codecov-io · 2017-07-19T21:15:30Z

Codecov Report

Merging #2111 into master will increase coverage by <.01%.
The diff coverage is 97.87%.

@@            Coverage Diff             @@
##           master    #2111      +/-   ##
==========================================
+ Coverage   97.09%   97.09%   +<.01%     
==========================================
  Files          38       39       +1     
  Lines        7740     7780      +40     
  Branches     1351     1357       +6     
==========================================
+ Hits         7515     7554      +39     
  Misses        101      101              
- Partials      124      125       +1

Impacted Files	Coverage Δ
aiohttp/connector.py	`97.65% <100%> (+0.07%)`	⬆️
aiohttp/locks.py	`96% <96%> (ø)`

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 44edff8...f138cde. Read the comment docs.

pfreixes · 2017-07-25T17:42:39Z

Any chances to keep the discussion that we had @asvetlov ? any chances to gather the opinion from other maintainers such as @fafhrd91 ?

fafhrd91 · 2017-07-25T20:21:21Z

you should to cleanup all event in connector's close method. just set CancelledError.

otherwise I think PR is fine.

pfreixes · 2017-07-26T15:15:56Z

aiohttp/connector.py

        self._ssl_context = ssl_context
        self._family = family
        self._local_addr = local_addr

+    def close(self):


@fafhrd91 and @asvetlov now the throttled requests are canceled if the connector is closed explicitly. Not really sure if I'm happy with the implementation, to make it possible I had to save all waiters and chain them with the waiters related to the Event, take a look alternatives are welcomed.

fafhrd91 · 2017-07-26T15:30:34Z

this is for 2.3 release. otherwise lgtm.

@asvetlov ?

pfreixes · 2017-07-30T07:32:29Z

@asvetlov looks you have the last word for this PR. BTW adapted to last CHANGES protocol and one small fix about the explicit loop.

kxepal · 2017-07-30T08:46:46Z

Please do rebase feature branch against master instead of merging master into feature branch. Twisted git log history is quite hard to follow.

asvetlov · 2017-07-30T09:34:16Z

@kxepal it doesn't needed, we usually do quash PRs anyway

kxepal · 2017-07-30T10:22:51Z

@asvetlov ah, cool then!

asvetlov · 2017-07-30T11:10:48Z

aiohttp/locks.py

+from .helpers import ensure_future
+
+
+class ErrorfulOneShotEvent:


The name is too long.
Also it starts with Error, usually this schema is used for nameing exception classes.

Changed to EventResultOrError, still long but at least more meaningful. Let me know if it suits better for you.

asvetlov · 2017-07-31T05:05:22Z

Ok, thanks. The name is not public, we could change it later anyway.

lock · 2019-10-28T20:03:59Z

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.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

pfreixes added 12 commits May 26, 2017 16:25

Explicit loop for sleep calls

3d2a05d

Fixed typos in documentation

3800002

Merge remote-tracking branch 'upstream/master' into throttle_dns_requ…

2c27b41

…ests

Added DNS request throttling in CHANGES.rst

2dc2486

Dogpile invalid word by doc-spelling

4139d24

Get full coverage for locks

89accd7

Merge branch 'master' into throttle_dns_requests

4f35784

Remove implicit loop tests for locks.Event

7e11ed4

Use create_future helper

2ab0132

Updated spelling wordlist

5586325

kxepal reviewed Jul 19, 2017

View reviewed changes

Use a simple Event wrapper instead of a full Event class implementation

11afb32

Pass the loop explicitly

8db6db1

Cancel pending throttled DNS requests if connector is close

a7704a5

pfreixes commented Jul 26, 2017

View reviewed changes

Removed not used attribute

517dfca

pfreixes added 3 commits July 30, 2017 09:24

Pass loop explicit

1eebd9a

Merge branch 'master' into throttle_dns_requests

e885b98

Throtlle DNS feature adapted to CHANGES protocol

fa5b68d

Update 2111.feature

5b1500b

asvetlov requested changes Jul 30, 2017

View reviewed changes

Changed class name

f138cde

asvetlov approved these changes Jul 31, 2017

View reviewed changes

asvetlov merged commit e7b62d1 into aio-libs:master Jul 31, 2017

lock bot added the outdated label Oct 28, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throttle simultaneous DNS requests #1924 #2111

Throttle simultaneous DNS requests #1924 #2111

pfreixes commented Jul 18, 2017

pfreixes commented Jul 18, 2017

fafhrd91 commented Jul 18, 2017 •

edited

Loading

kxepal Jul 19, 2017

pfreixes Jul 19, 2017

pfreixes commented Jul 19, 2017

codecov-io commented Jul 19, 2017 •

edited

Loading

pfreixes commented Jul 25, 2017

fafhrd91 commented Jul 25, 2017

pfreixes Jul 26, 2017

fafhrd91 commented Jul 26, 2017

pfreixes commented Jul 30, 2017

kxepal commented Jul 30, 2017

asvetlov commented Jul 30, 2017

kxepal commented Jul 30, 2017

asvetlov Jul 30, 2017

pfreixes Jul 30, 2017

asvetlov commented Jul 31, 2017

lock bot commented Oct 28, 2019

		from .helpers import ensure_future


		class ErrorfulOneShotEvent:

Throttle simultaneous DNS requests #1924 #2111

Throttle simultaneous DNS requests #1924 #2111

Conversation

pfreixes commented Jul 18, 2017

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

pfreixes commented Jul 18, 2017

fafhrd91 commented Jul 18, 2017 • edited Loading

kxepal Jul 19, 2017

Choose a reason for hiding this comment

pfreixes Jul 19, 2017

Choose a reason for hiding this comment

pfreixes commented Jul 19, 2017

codecov-io commented Jul 19, 2017 • edited Loading

Codecov Report

pfreixes commented Jul 25, 2017

fafhrd91 commented Jul 25, 2017

pfreixes Jul 26, 2017

Choose a reason for hiding this comment

fafhrd91 commented Jul 26, 2017

pfreixes commented Jul 30, 2017

kxepal commented Jul 30, 2017

asvetlov commented Jul 30, 2017

kxepal commented Jul 30, 2017

asvetlov Jul 30, 2017

Choose a reason for hiding this comment

pfreixes Jul 30, 2017

Choose a reason for hiding this comment

asvetlov commented Jul 31, 2017

lock bot commented Oct 28, 2019

fafhrd91 commented Jul 18, 2017 •

edited

Loading

codecov-io commented Jul 19, 2017 •

edited

Loading