Unit test for _pick_a_http_server.

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)
         )

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)