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

@@ -6,7 +6,8 @@
 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
@@ -206,9 +207,9 @@ def measure_relay(args, conf, destinations, cb, rl, relay):
     log.debug('Built circ %s %s for relay %s %s', circ_id,
               stem_utils.circuit_str(cb.controller, circ_id), relay.nickname,
               relay.fingerprint[0:8])
-    # 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.warning('When measuring %s %s the destination seemed to have '
                     'stopped being usable: %s', relay.nickname,

sbws/lib/destination.py

@@ -159,7 +159,8 @@ def __init__(self, conf, dests, circuit_builder, relay_list, controller):
         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')
@@ -168,8 +169,10 @@ def __init__(self, conf, dests, circuit_builder, relay_list, controller):
         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()
@@ -245,23 +248,10 @@ def next(self):
         '''
         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)

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