commit 331406cb5ffe8e4e216e6d1396664f34d6c3cb28 Author: Damian Johnson atagar@torproject.org Date: Thu Apr 19 12:12:15 2018 -0700
Invalidate cached address and exit policy when our address changes
Great point from dmr and others on #25423 that when our address changes we need to invalidate our cached exit policy. STATUS_SERVER events provide us with a notification when our address changes.
Besides our exit policy, this also lets us simplify our caching around addresses. Stem had a special TTL so we wouldn't cache invalid addresses indefinitely, but with this change we can now replace that with event driven invalidation (much better!). --- stem/control.py | 22 +++++++++------------- test/unit/control/controller.py | 20 +++++++++++++------- 2 files changed, 22 insertions(+), 20 deletions(-)
diff --git a/stem/control.py b/stem/control.py index 02183d95..ecec96aa 100644 --- a/stem/control.py +++ b/stem/control.py @@ -389,12 +389,6 @@ CACHEABLE_GETINFO_PARAMS_UNTIL_SETCONF = ( 'accounting/enabled', )
-# 'GETINFO address' isn't technically cachable, but it's also both highly -# requested and highly static. As such fetching it at a fixed rate. If -# you'd like you can set this global to zero to disable caching. - -CACHE_ADDRESS_FOR = 1.0 - # GETCONF parameters we shouldn't cache. This includes hidden service # perameters due to the funky way they're set and retrieved (for instance, # 'SETCONF HiddenServiceDir' effects 'GETCONF HiddenServiceOptions'). @@ -1055,7 +1049,6 @@ class Controller(BaseController): self._enabled_features = [] self._is_geoip_unavailable = None
- self._address_cached_at = 0 self._last_address_exc = None self._last_fingerprint_exc = None
@@ -1076,6 +1069,14 @@ class Controller(BaseController):
self.add_event_listener(_confchanged_listener, EventType.CONF_CHANGED)
+ def _address_changed_listener(event): + if event.action == 'EXTERNAL_ADDRESS': + self._set_cache({'exit_policy': None}) + self._set_cache({'address': None}, 'getinfo') + self._last_address_exc = None + + self.add_event_listener(_address_changed_listener, EventType.STATUS_SERVER) + def close(self): self.clear_cache() super(Controller, self).close() @@ -1153,7 +1154,7 @@ class Controller(BaseController): for param in params: if param.startswith('ip-to-country/') and param != 'ip-to-country/0.0.0.0' and self.is_geoip_unavailable(): raise stem.ProtocolError('Tor geoip database is unavailable') - elif param == 'address' and self._last_address_exc and (time.time() - self._address_cached_at) < CACHE_ADDRESS_FOR: + elif param == 'address' and self._last_address_exc: raise self._last_address_exc # we already know we can't resolve an address elif param == 'fingerprint' and self._last_fingerprint_exc and self.get_conf('ORPort', None) is None: raise self._last_fingerprint_exc # we already know we're not a relay @@ -1164,9 +1165,6 @@ class Controller(BaseController): cached_results = self._get_cache_map(from_cache, 'getinfo')
for key in cached_results: - if key == 'address' and (time.time() - self._address_cached_at) > CACHE_ADDRESS_FOR: - continue # cached address is too old - user_expected_key = _case_insensitive_lookup(params, key) reply[user_expected_key] = cached_results[key] params.remove(user_expected_key) @@ -1207,7 +1205,6 @@ class Controller(BaseController): self._set_cache(to_cache, 'getinfo')
if 'address' in params: - self._address_cached_at = time.time() self._last_address_exc = None
if 'fingerprint' in params: @@ -1221,7 +1218,6 @@ class Controller(BaseController): return list(reply.values())[0] except stem.ControllerError as exc: if 'address' in params: - self._address_cached_at = time.time() self._last_address_exc = exc
if 'fingerprint' in params: diff --git a/test/unit/control/controller.py b/test/unit/control/controller.py index 92abfb67..e7b014b9 100644 --- a/test/unit/control/controller.py +++ b/test/unit/control/controller.py @@ -5,7 +5,6 @@ integ tests, but a few bits lend themselves to unit testing.
import datetime import io -import time import unittest
import stem.descriptor.router_status_entry @@ -32,7 +31,10 @@ class TestControl(unittest.TestCase): def setUp(self): socket = stem.socket.ControlSocket()
- with patch('stem.control.Controller.add_event_listener', Mock()): + # When initially constructing a controller we need to suppress msg, so our + # constructor's SETEVENTS requests pass. + + with patch('stem.control.BaseController.msg', Mock()): self.controller = Controller(socket)
def test_event_description(self): @@ -53,13 +55,10 @@ class TestControl(unittest.TestCase):
@patch('stem.control.Controller.msg') def test_get_info_address_caching(self, msg_mock): - test_start = time.time() msg_mock.return_value = ControlMessage.from_str('551 Address unknown\r\n')
- self.assertEqual(0, self.controller._address_cached_at) self.assertEqual(None, self.controller._last_address_exc) self.assertRaisesRegexp(stem.ProtocolError, 'Address unknown', self.controller.get_info, 'address') - self.assertTrue(test_start <= self.controller._address_cached_at <= time.time()) self.assertEqual("GETINFO response didn't have an OK status:\nAddress unknown", str(self.controller._last_address_exc)) self.assertEqual(1, msg_mock.call_count)
@@ -68,12 +67,19 @@ class TestControl(unittest.TestCase): self.assertRaisesRegexp(stem.ProtocolError, 'Address unknown', self.controller.get_info, 'address') self.assertEqual(1, msg_mock.call_count)
- # pretend it's a minute later and try again, now with an address + # invalidates the cache, transitioning from no address to having one
msg_mock.return_value = ControlMessage.from_str('250-address=17.2.89.80\r\n250 OK\r\n', 'GETINFO') self.assertRaisesRegexp(stem.ProtocolError, 'Address unknown', self.controller.get_info, 'address') - self.controller._address_cached_at -= 60 + self.controller._handle_event(ControlMessage.from_str('650 STATUS_SERVER NOTICE EXTERNAL_ADDRESS ADDRESS=17.2.89.80 METHOD=DIRSERV\r\n')) + self.assertEqual('17.2.89.80', self.controller.get_info('address')) + + # invalidates the cache, transitioning from one address to another + + msg_mock.return_value = ControlMessage.from_str('250-address=80.89.2.17\r\n250 OK\r\n', 'GETINFO') self.assertEqual('17.2.89.80', self.controller.get_info('address')) + self.controller._handle_event(ControlMessage.from_str('650 STATUS_SERVER NOTICE EXTERNAL_ADDRESS ADDRESS=80.89.2.17 METHOD=DIRSERV\r\n')) + self.assertEqual('80.89.2.17', self.controller.get_info('address'))
@patch('stem.control.Controller.msg') @patch('stem.control.Controller.get_conf')