From 32768e310ae5316cc2e6fc0f0f969368c1ff3eee Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 21 Feb 2023 11:30:47 -0500 Subject: [PATCH] Unit test for _pick_a_http_server. --- src/allmydata/storage_client.py | 48 ++++++++++------- src/allmydata/test/test_storage_client.py | 64 ++++++++++++++++++++++- 2 files changed, 93 insertions(+), 19 deletions(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 8191014e8..436335431 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -33,7 +33,7 @@ Ported to Python 3. from __future__ import annotations from six import ensure_text -from typing import Union +from typing import Union, Callable, Any import re, time, hashlib from os import urandom from configparser import NoSectionError @@ -935,19 +935,25 @@ class NativeStorageServer(service.MultiService): self._reconnector.reset() -async def _pick_a_http_server(reactor, nurls: list[DecodedURL]) -> DecodedURL: - """Pick the first server we successfully talk to.""" +async def _pick_a_http_server( + reactor, + nurls: list[DecodedURL], + request: Callable[[DecodedURL, Any], defer.Deferred[Any]] +) -> DecodedURL: + """Pick the first server we successfully send a request to.""" while True: - try: - _, index = await defer.DeferredList([ - StorageClientGeneral( - StorageClient.from_nurl(nurl, reactor) - ).get_version() for nurl in nurls - ], consumeErrors=True, fireOnOneCallback=True) + result = await defer.DeferredList([ + request(reactor, nurl) for nurl in nurls + ], consumeErrors=True, fireOnOneCallback=True) + # Apparently DeferredList is an awful awful API. If everything fails, + # you get back a list of (False, Failure), if it succeeds, you get a + # tuple of (value, index). + if isinstance(result, list): + await deferLater(reactor, 1, lambda: None) + else: + assert isinstance(result, tuple) + _, index = result return nurls[index] - except Exception as e: - log.err(e, "Failed to connect to any of the HTTP NURLs for server") - await deferLater(reactor, 1, lambda: None) @implementer(IServer) @@ -975,9 +981,10 @@ class HTTPNativeStorageServer(service.MultiService): self._short_description, self._long_description ) = _parse_announcement(server_id, furl, announcement) - # TODO need some way to do equivalent of Happy Eyeballs for multiple NURLs? - # https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3935 - self._nurls = [DecodedURL.from_text(u) for u in announcement[ANONYMOUS_STORAGE_NURLS]] + self._nurls = [ + DecodedURL.from_text(u) + for u in announcement[ANONYMOUS_STORAGE_NURLS] + ] self._istorage_server = None self._connection_status = connection_status.ConnectionStatus.unstarted() @@ -1048,9 +1055,14 @@ class HTTPNativeStorageServer(service.MultiService): @async_to_deferred async def start_connecting(self, trigger_cb): - # The problem with this scheme is that while picking the HTTP server to - # talk to, we don't have connection status updates... - nurl = await _pick_a_http_server(reactor, self._nurls) + # TODO file a bug: The problem with this scheme is that while picking + # the HTTP server to talk to, we don't have connection status + # updates... + def request(reactor, nurl: DecodedURL): + return StorageClientGeneral( + StorageClient.from_nurl(nurl, reactor) + ).get_version() + nurl = await _pick_a_http_server(reactor, self._nurls, request) self._istorage_server = _HTTPStorageServer.from_http_client( StorageClient.from_nurl(nurl, reactor) ) diff --git a/src/allmydata/test/test_storage_client.py b/src/allmydata/test/test_storage_client.py index 0657a814e..38ef8c1d3 100644 --- a/src/allmydata/test/test_storage_client.py +++ b/src/allmydata/test/test_storage_client.py @@ -7,8 +7,10 @@ from __future__ import annotations from json import ( loads, ) - import hashlib +from typing import Union, Any + +from hyperlink import DecodedURL from fixtures import ( TempDir, ) @@ -52,6 +54,7 @@ from twisted.internet.defer import ( from twisted.python.filepath import ( FilePath, ) +from twisted.internet.task import Clock from foolscap.api import ( Tub, @@ -80,12 +83,14 @@ from allmydata.webish import ( WebishServer, ) from allmydata.util import base32, yamlutil +from allmydata.util.deferredutil import async_to_deferred from allmydata.storage_client import ( IFoolscapStorageServer, NativeStorageServer, StorageFarmBroker, _FoolscapStorage, _NullStorage, + _pick_a_http_server ) from ..storage.server import ( StorageServer, @@ -731,3 +736,60 @@ storage: yield done self.assertTrue(done.called) + + +class PickHTTPServerTests(unittest.SynchronousTestCase): + """Tests for ``_pick_a_http_server``.""" + + def loop_until_result(self, url_to_results: dict[DecodedURL, list[tuple[float, Union[Exception, Any]]]]) -> Deferred[DecodedURL]: + """ + Given mapping of URLs to list of (delay, result), return the URL of the + first selected server. + """ + clock = Clock() + + def request(reactor, url): + delay, value = url_to_results[url].pop(0) + result = Deferred() + def add_result_value(): + if isinstance(value, Exception): + result.errback(value) + else: + result.callback(value) + reactor.callLater(delay, add_result_value) + return result + + d = async_to_deferred(_pick_a_http_server)( + clock, list(url_to_results.keys()), request + ) + for i in range(1000): + clock.advance(0.1) + return d + + def test_first_successful_connect_is_picked(self): + """ + Given multiple good URLs, the first one that connects is chosen. + """ + earliest_url = DecodedURL.from_text("http://a") + latest_url = DecodedURL.from_text("http://b") + d = self.loop_until_result({ + latest_url: [(2, None)], + earliest_url: [(1, None)] + }) + self.assertEqual(self.successResultOf(d), earliest_url) + + def test_failures_are_retried(self): + """ + If the initial requests all fail, ``_pick_a_http_server`` keeps trying + until success. + """ + eventually_good_url = DecodedURL.from_text("http://good") + bad_url = DecodedURL.from_text("http://bad") + d = self.loop_until_result({ + eventually_good_url: [ + (1, ZeroDivisionError()), (0.1, ZeroDivisionError()), (1, None) + ], + bad_url: [(0.1, RuntimeError()), (0.1, RuntimeError()), (0.1, RuntimeError())] + }) + self.flushLoggedErrors(ZeroDivisionError, RuntimeError) + self.assertEqual(self.successResultOf(d), eventually_good_url)