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

Connector caching #415

Merged
merged 9 commits into from
Jun 20, 2015
Merged
Show file tree
Hide file tree
Changes from 4 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
67 changes: 54 additions & 13 deletions aiohttp/connector.py
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,8 @@ def _create_connection(self, req):
_SSL_OP_NO_COMPRESSION = getattr(ssl, "OP_NO_COMPRESSION", 0)
_SSH_HAS_CREATE_DEFAULT_CONTEXT = hasattr(ssl, 'create_default_context')

_marker = object()


class TCPConnector(BaseConnector):
"""TCP connector.
Expand All @@ -400,7 +402,8 @@ class TCPConnector(BaseConnector):
"""

def __init__(self, *, verify_ssl=True, fingerprint=None,
resolve=False, family=socket.AF_INET, ssl_context=None,
resolve=_marker, cache_dns=_marker,
family=socket.AF_INET, ssl_context=None,
**kwargs):
super().__init__(**kwargs)

Expand All @@ -419,10 +422,21 @@ def __init__(self, *, verify_ssl=True, fingerprint=None,
self._hashfunc = hashfunc
self._fingerprint = fingerprint

if cache_dns is not _marker and resolve is not _marker:
if cache_dns != resolve:
raise ValueError("cashe_dns must agree with resolve")
Copy link
Member

Choose a reason for hiding this comment

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

s/cashe/cache/

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

_cache_dns = cache_dns
elif cache_dns is not _marker:
_cache_dns = cache_dns
elif resolve is not _marker:
_cache_dns = resolve
else:
_cache_dns = False

self._cache_dns = _cache_dns
self._cached_hosts = {}
self._ssl_context = ssl_context
self._family = family
self._resolve = resolve
self._resolved_hosts = {}

@property
def verify_ssl(self):
Expand Down Expand Up @@ -466,31 +480,58 @@ def family(self):
"""Socket family like AF_INET."""
return self._family

@property
def cache_dns(self):
Copy link
Member

Choose a reason for hiding this comment

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

cache_dns assumes some action, here is "to cache dns". But for property either dns_cache or cached_dns are better names.

Copy link
Member

Choose a reason for hiding this comment

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

Especially since below you use "cached_hosts" and "clear_dns_cache" terminology.

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed cache_dns to dns_cache.
Do you suggest renaming ctor parameter also?

Copy link
Member

Choose a reason for hiding this comment

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

Worth to. Btw, on second thought I found myself incorrect about the name: seems like this property is used as True/False flag in order to define will dns cache be used or not, right? Then use_dns_cache should be semantically better and won't cause confusion about if it some sort of cache mapping or not. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

use_dns_cache is the best, sure.
Thanks for suggestion

On Sat, Jun 20, 2015 at 4:37 PM, Alexander Shorin [email protected]
wrote:

In aiohttp/connector.py
#415 (comment):

@@ -467,30 +481,57 @@ def family(self):
return self._family

 @property
  • def cache_dns(self):

Worth to. Btw, on second thought I found myself incorrect about the name:
seems like this property is used as True/False flag in order to define will
dns cache be used or not, right? Then use_dns_cache should be
semantically better and won't cause confusion about if it some sort of
cache mapping or not. What do you think?


Reply to this email directly or view it on GitHub
https://github.com/KeepSafe/aiohttp/pull/415/files#r32884576.

Thanks,
Andrew Svetlov

"""True if local DNS caching is enabled."""
return self._cache_dns

@property
def cached_hosts(self):
"""Read-only dict of cached DNS record."""
return MappingProxyType(self._cached_hosts)

def clear_dns_cache(self, host=None, port=None):
"""Remove specified host/port or clear all dns local cache."""
if host is not None and port is not None:
self._cached_hosts.pop((host, port), None)
elif host is not None or port is not None:
raise ValueError("either both host and port "
"or none of them are allowed")
else:
self._cached_hosts.clear()

@property
def resolve(self):
"""Do DNS lookup for host name?"""
return self._resolve
warnings.warn((".resolve property is deprecated, "
Copy link
Member

Choose a reason for hiding this comment

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

If resolve is deprecated, may be raise warning in __init__ as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

"use .cache_dns instead"),
DeprecationWarning, stacklevel=2)
return self.cache_dns

@property
def resolved_hosts(self):
"""The dict of (host, port) -> (ipaddr, port) pairs."""
return MappingProxyType(self._resolved_hosts)
warnings.warn((".resolved_hosts property is deprecated, "
"use .cached_hosts instead"),
DeprecationWarning, stacklevel=2)
return self.cached_hosts

def clear_resolved_hosts(self, host=None, port=None):
"""Remove specified host/port or clear all resolve cache."""
warnings.warn((".clear_resolved_hosts() is deprecated, "
"use .clear_dns_cache() instead"),
DeprecationWarning, stacklevel=2)
if host is not None and port is not None:
key = (host, port)
if key in self._resolved_hosts:
del self._resolved_hosts[key]
self.clear_dns_cache(host, port)
else:
self._resolved_hosts.clear()
self.clear_dns_cache()

@asyncio.coroutine
def _resolve_host(self, host, port):
if self._resolve:
if self._cache_dns:
key = (host, port)

if key not in self._resolved_hosts:
if key not in self._cached_hosts:
infos = yield from self._loop.getaddrinfo(
host, port, type=socket.SOCK_STREAM, family=self._family)

Expand All @@ -501,9 +542,9 @@ def _resolve_host(self, host, port):
'host': address[0], 'port': address[1],
'family': family, 'proto': proto,
'flags': socket.AI_NUMERICHOST})
self._resolved_hosts[key] = hosts
self._cached_hosts[key] = hosts

return list(self._resolved_hosts[key])
return list(self._cached_hosts[key])
else:
return [{'hostname': host, 'host': host, 'port': port,
'family': self._family, 'proto': 0, 'flags': 0}]
Expand Down
53 changes: 49 additions & 4 deletions tests/test_connector.py
Original file line number Diff line number Diff line change
Expand Up @@ -458,8 +458,15 @@ def test_tcp_connector_ctor(self):
conn = aiohttp.TCPConnector(loop=self.loop)
self.assertTrue(conn.verify_ssl)
self.assertIs(conn.fingerprint, None)
self.assertFalse(conn.resolve)

with self.assertWarns(DeprecationWarning):
self.assertFalse(conn.resolve)
self.assertFalse(conn.cache_dns)

self.assertEqual(conn.family, socket.AF_INET)

with self.assertWarns(DeprecationWarning):
self.assertEqual(conn.resolved_hosts, {})
self.assertEqual(conn.resolved_hosts, {})

def test_tcp_connector_ctor_fingerprint_valid(self):
Expand Down Expand Up @@ -514,17 +521,37 @@ def test_tcp_connector_fingerprint(self):
def test_tcp_connector_clear_resolved_hosts(self):
conn = aiohttp.TCPConnector(loop=self.loop)
info = object()
conn._resolved_hosts[('localhost', 123)] = info
conn._resolved_hosts[('localhost', 124)] = info
conn._cached_hosts[('localhost', 123)] = info
conn._cached_hosts[('localhost', 124)] = info
conn.clear_resolved_hosts('localhost', 123)
self.assertEqual(
conn.resolved_hosts, {('localhost', 124): info})
conn.clear_resolved_hosts('localhost', 123)
self.assertEqual(
conn.resolved_hosts, {('localhost', 124): info})
conn.clear_resolved_hosts()
with self.assertWarns(DeprecationWarning):
conn.clear_resolved_hosts()
self.assertEqual(conn.resolved_hosts, {})

def test_tcp_connector_clear_dns_cache(self):
conn = aiohttp.TCPConnector(loop=self.loop)
info = object()
conn._cached_hosts[('localhost', 123)] = info
conn._cached_hosts[('localhost', 124)] = info
conn.clear_dns_cache('localhost', 123)
self.assertEqual(
conn.cached_hosts, {('localhost', 124): info})
conn.clear_dns_cache('localhost', 123)
self.assertEqual(
conn.cached_hosts, {('localhost', 124): info})
conn.clear_dns_cache()
self.assertEqual(conn.cached_hosts, {})

def test_tcp_connector_clear_dns_cache_bad_args(self):
conn = aiohttp.TCPConnector(loop=self.loop)
with self.assertRaises(ValueError):
conn.clear_dns_cache('localhost')

def test_ambigous_verify_ssl_and_ssl_context(self):
with self.assertRaises(ValueError):
aiohttp.TCPConnector(
Expand Down Expand Up @@ -756,6 +783,24 @@ def test_connector_cookie_deprecation(self):
conn = aiohttp.TCPConnector(share_cookies=True, loop=self.loop)
conn.close()

def test_ambiguous_ctor_params(self):
with self.assertRaises(ValueError):
aiohttp.TCPConnector(resolve=True, cache_dns=False, loop=self.loop)

def test_both_resolve_and_cache_dns(self):
conn = aiohttp.TCPConnector(resolve=True, cache_dns=True,
loop=self.loop)
self.assertTrue(conn.cache_dns)
with self.assertWarns(DeprecationWarning):
self.assertTrue(conn.resolve)

def test_both_cache_dns_only(self):
conn = aiohttp.TCPConnector(cache_dns=True,
loop=self.loop)
self.assertTrue(conn.cache_dns)
with self.assertWarns(DeprecationWarning):
self.assertTrue(conn.resolve)


class TestProxyConnector(unittest.TestCase):

Expand Down