Add new ttl option to expire DNS entries after N seconds

[pfreixes]

What do these changes do?

Add a new option to expire DNS cached entries at some point, this will avoid having invalid DNS entries for those environments where translations changes in some time policy.

Are there changes in behavior for the user?

By default it behaves as backward compatibility, entries are cached forever. Using the new ttl_dns_cache parameter the user can specify how many seconds a DNS entry will be keep into the table before to be refreshed.

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.

[codecov-io]

Codecov Report

Merging #1819 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1819      +/-   ##
==========================================
+ Coverage   97.18%   97.23%   +0.04%     
==========================================
  Files          37       37              
  Lines        7467     7478      +11     
  Branches     1297     1299       +2     
==========================================
+ Hits         7257     7271      +14     
+ Misses         89       87       -2     
+ Partials      121      120       -1

Impacted Files	Coverage Δ
aiohttp/connector.py	97.44% <100%> (+0.76%)	⬆️
aiohttp/helpers.py	97.59% <0%> (+0.01%)	⬆️

---

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 0427370...25f51ee. Read the comment docs.

[ludovic-gasc]

Hi @pfreixes,

Normally, you have a ttl at server side for each DNS record.
Are you sure it's kept in cache forever or the ttl is respected ?

Thanks for your answer.

[pfreixes]

Hi @GMLudo,

Unfortunately, retrieve the TTL as a related metadata of a DNS query can end up being a mess, the default implementation of getaddrinfo relays on the C implementation that implements for POSIX systems the most common RFC, including as a response a limited structure [1]. I would say that using aiodns and avoiding the naive gethostbyname implementation and using the raw access to the cares via the query function it should be possible.

IMHO the current commit avoids all of this complexity just adding a new user parameter, with that the user has the chance to avoid having wrong entries in his DNS cache and also taking advantage of the cache yet. In fact, I would prefer to put some value such as 60 seconds per default ... but it could create too much controversial because of breaking change.

There is a discussion also open in the Tornado repo about that [2]

Yes, If I'm not wrong a Session caches the entries forever. Use the same session for a long time can end up having DNS entries that are not valid, perhaps if the services are behind an AWS ELB [3]

[1] http://man7.org/linux/man-pages/man3/getaddrinfo.3.html
[2] tornadoweb/tornado#626
[3] https://aws.amazon.com/articles/1636185810492479

[fafhrd91]

could you use loop.time() instead of monotonic.

[fafhrd91]

I like your changes. could you just use loop.time() instead of monotonic.

[fafhrd91]

some more notes:

1. let's use some default value, something like 10
2. also could you add doc for this parameter to client_reference.rst
3. add yourself to contributors list

[ludovic-gasc]

Hi @pfreixes,

Thanks for your explanations, it's now clear to me, and thanks for your pull request BTW ;-)

Have a nice week.

[pfreixes]

@fafhrd91 done. The documentation was already there and I've contributed before and I was already in at CONTRIBUTORS.rst file

FYI This week I'm gonna work in give support for multiple hosts address per hostname, it might mean some changes in TCPConnector and the DNS resolvers. The question is how backward compatibility has to be the cached_hosts property [1]? I didn't want put the timestamp and the address as a unique value to avoid break the data type exposed by this property, but the changes behind the commit related to give support for multiple support might need to change the string type to something like a set. What do you think ?

[1] https://github.com/aio-libs/aiohttp/blob/master/aiohttp/connector.py#L590

[pfreixes]

I've renamed a bit the unit tests about the DNS stuff, just let me know if you are happier with the new approx.

[fafhrd91]

change it to 2.1

[fafhrd91]

thanks!

[lock]

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.