commit d33ac8f6d23b8b30664311b205a87d9a55e4c1d1 Author: juga0 <juga@riseup.net> Date: Tue Dec 18 11:33:54 2018 +0000 destination: stop running twice usability tests in every measurement. This removes the need for an extra lock for every measurement It should also not be depending on a time interval, but on the number of failures detected. Not counting number of failures since it would need to modify the destination or list of at runtime. It should be done in a future refactor. Fixes bug #28897. Bugfix v0.3.0 --- sbws/core/scanner.py | 9 +++-- sbws/lib/destination.py | 36 +++++++----------- tests/integration/lib/test_destination.py | 61 +++++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 27 deletions(-) diff --git a/sbws/core/scanner.py b/sbws/core/scanner.py index 78cc165..dd4198c 100644 --- a/sbws/core/scanner.py +++ b/sbws/core/scanner.py @@ -11,7 +11,8 @@ from ..lib.resultdump import ResultSuccess, ResultErrorCircuit from ..lib.resultdump import ResultErrorStream from ..lib.relaylist import RelayList from ..lib.relayprioritizer import RelayPrioritizer -from ..lib.destination import DestinationList +from ..lib.destination import (DestinationList, + connect_to_destination_over_circuit) from ..util.timestamp import now_isodt_str from ..util.state import State from sbws.globals import fail_hard, HTTP_GET_HEADERS, TIMEOUT_MEASUREMENTS @@ -284,9 +285,9 @@ def measure_relay(args, conf, destinations, cb, rl, relay): ] log.debug('Built circuit with path %s (%s) to measure %s (%s)', circ_fps, nicknames, relay.fingerprint, relay.nickname) - # Make a connection to the destionation webserver and make sure it can - # still help us measure - is_usable, usable_data = dest.is_usable(circ_id, s, cb.controller) + # Make a connection to the destination + is_usable, usable_data = connect_to_destination_over_circuit( + dest, circ_id, s, cb.controller, dest._max_dl) if not is_usable: log.debug('Destination %s unusable via circuit %s (%s), %s', dest.url, circ_fps, nicknames, usable_data) diff --git a/sbws/lib/destination.py b/sbws/lib/destination.py index 7bd64e8..468b3f6 100644 --- a/sbws/lib/destination.py +++ b/sbws/lib/destination.py @@ -163,7 +163,8 @@ class DestinationList: self._cb = circuit_builder self._rl = relay_list self._all_dests = dests - self._usable_dests = [] + # Inially all destionations are usable until proven otherwise. + self._usable_dests = self._all_dests self._last_usability_test = 0 self._usability_test_interval = \ conf.getint('destinations', 'usability_test_interval') @@ -172,8 +173,10 @@ class DestinationList: self._usability_lock = RLock() def _should_perform_usability_test(self): - return self._last_usability_test + self._usability_test_interval <\ - time.time() + # Until bigger refactor, do not perform usability test. + # Every time a measurement is done, it already performs what usability + # test does. + return False def _perform_usability_test(self): self._usability_lock.acquire() @@ -250,23 +253,10 @@ class DestinationList: ''' Returns the next destination that should be used in a measurement ''' - with self._usability_lock: - while True: - if self._should_perform_usability_test(): - self._perform_usability_test() - log.debug('%s/%s of our configured destinations are ' - 'usable at this time', len(self._usable_dests), - len(self._all_dests)) - if len(self._usable_dests) > 0: - break - time_till_next_check = self._usability_test_interval + 0.0001 - log.warning( - 'Of our %d configured destinations, none are usable at ' - 'this time. Sleeping %f seconds on this blocking call ' - 'to DestinationList.next() until we can check for a ' - 'usable destination again.', len(self._all_dests), - time_till_next_check) - time.sleep(time_till_next_check) - - self._rng.shuffle(self._usable_dests) - return self._usable_dests[0] + # Do not perform usability tests since a destination is already proven + # usable or not in every measurement, and it should depend on a X + # number of failures. + # This removes the need for an extra lock for every measurement. + # Do not change the order of the destinations, just return a + # destination. + return self._rng.choice(self._usable_dests) diff --git a/tests/integration/lib/test_destination.py b/tests/integration/lib/test_destination.py new file mode 100644 index 0000000..53c0ab5 --- /dev/null +++ b/tests/integration/lib/test_destination.py @@ -0,0 +1,61 @@ +"""Integration tests for destination.py""" +import sbws.util.requests as requests_utils +from sbws.lib.destination import (DestinationList, Destination, + connect_to_destination_over_circuit) + + +def test_destination_list_no_usability_test_success( + conf, persistent_launch_tor, cb, rl + ): + # In a future refactor, if DestionationList is not initialized with the + # controller, this test should be an unit test. + destination_list, error_msg = DestinationList.from_config( + conf, cb, rl, persistent_launch_tor + ) + # Initially all destinations should be "usable". + assert destination_list._all_dests == destination_list._usable_dests + # Because this is disabled. + assert destination_list._should_perform_usability_test() is False + # Because there's only 1 destination in conftest, random should return + # the same one. + assert destination_list.next() == \ + destination_list._all_dests[0] + + +def test_connect_to_destination_over_circuit_success(persistent_launch_tor, + dests, cb, rl): + destination = dests.next() + session = requests_utils.make_session(persistent_launch_tor, 10) + # Choose a relay that is not an exit + relay = [r for r in rl.relays + if r.nickname == 'relay1mbyteMAB'][0] + # Choose an exit, for this test it does not matter the bandwidth + helper = rl.exits_not_bad_allowing_port(destination.port)[0] + circuit_path = [relay.fingerprint, helper.fingerprint] + # build a circuit + circuit_id = cb.build_circuit(circuit_path) + # Perform "usability test" + is_usable, response = connect_to_destination_over_circuit( + destination, circuit_id, session, persistent_launch_tor, 1024) + assert is_usable is True + assert 'content_length' in response + + +def test_connect_to_destination_over_circuit_fail(persistent_launch_tor, + dests, cb, rl): + bad_destination = Destination('https://example.example', 1024, False) + # dests._all_dests.append(bad_destination) + # dests._usable_dests.append(bad_destination) + session = requests_utils.make_session(persistent_launch_tor, 10) + # Choose a relay that is not an exit + relay = [r for r in rl.relays + if r.nickname == 'relay1mbyteMAB'][0] + # Choose an exit, for this test it does not matter the bandwidth + helper = rl.exits_not_bad_allowing_port(bad_destination.port)[0] + circuit_path = [relay.fingerprint, helper.fingerprint] + # Build a circuit. + circuit_id = cb.build_circuit(circuit_path) + # Perform "usability test" + is_usable, response = connect_to_destination_over_circuit( + bad_destination, circuit_id, session, persistent_launch_tor, 1024) + assert is_usable is False