From 0da059b64486642864c0dbd211ccdb98909d79c1 Mon Sep 17 00:00:00 2001
From: Itamar Turner-Trauring <itamar@pythonspeed.com>
Date: Fri, 24 Mar 2023 10:10:18 -0400
Subject: [PATCH] Update the connection status during the initial choice of
 NURLs.

---
 src/allmydata/storage_client.py           | 50 ++++++++++++-----------
 src/allmydata/test/test_storage_client.py | 23 +++++++----
 2 files changed, 41 insertions(+), 32 deletions(-)

diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py
index c88613803..f71931c8b 100644
--- a/src/allmydata/storage_client.py
+++ b/src/allmydata/storage_client.py
@@ -1024,7 +1024,7 @@ async def _pick_a_http_server(
         reactor,
         nurls: list[DecodedURL],
         request: Callable[[Any, DecodedURL], defer.Deferred[Any]]
-) -> Optional[DecodedURL]:
+) -> DecodedURL:
     """Pick the first server we successfully send a request to.
 
     Fires with ``None`` if no server was found, or with the ``DecodedURL`` of
@@ -1035,18 +1035,8 @@ async def _pick_a_http_server(
         for nurl in nurls
     ])
 
-    try:
-        _, nurl = await queries
-        return nurl
-    except Exception as e:
-        # Logging errors breaks a bunch of tests, and it's not a _bug_ to
-        # have a failed connection, it's often expected and transient. More
-        # of a warning, really?
-        log.msg(
-            "Failed to connect to a storage server advertised by NURL: {}".format(
-                e)
-        )
-        return None
+    _, nurl = await queries
+    return nurl
 
 
 @implementer(IServer)
@@ -1223,19 +1213,31 @@ class HTTPNativeStorageServer(service.MultiService):
             picking = _pick_a_http_server(reactor, self._nurls, request)
             self._connecting_deferred = picking
             try:
-                nurl = await picking
-            finally:
-                self._connecting_deferred = None
-
-            if nurl is None:
-                # We failed to find a server to connect to. Perhaps the next
-                # iteration of the loop will succeed.
-                return
-            else:
-                self._istorage_server = _HTTPStorageServer.from_http_client(
-                    StorageClient.from_nurl(nurl, reactor)
+                try:
+                    nurl = await picking
+                finally:
+                    self._connecting_deferred = None
+            except Exception as e:
+                # Logging errors breaks a bunch of tests, and it's not a _bug_ to
+                # have a failed connection, it's often expected and transient. More
+                # of a warning, really?
+                log.msg(
+                    "Failed to connect to a storage server advertised by NURL: {}".format(e)
                 )
 
+                # Update the connection status:
+                self._failed_to_connect(Failure(e))
+
+                # Since we failed to find a server to connect to, give up
+                # for now. Perhaps the next iteration of the loop will
+                # succeed.
+                return
+
+            # iF we've gotten this far, we've found a working NURL.
+            self._istorage_server = _HTTPStorageServer.from_http_client(
+                StorageClient.from_nurl(nurl, reactor)
+            )
+
         result = self._istorage_server.get_version()
 
         def remove_connecting_deferred(result):
diff --git a/src/allmydata/test/test_storage_client.py b/src/allmydata/test/test_storage_client.py
index 91668e7ca..cf4a939e8 100644
--- a/src/allmydata/test/test_storage_client.py
+++ b/src/allmydata/test/test_storage_client.py
@@ -63,6 +63,8 @@ from foolscap.ipb import (
     IConnectionHintHandler,
 )
 
+from allmydata.util.deferredutil import MultiFailure
+
 from .no_network import LocalWrapper
 from .common import (
     EMPTY_CLIENT_CONFIG,
@@ -782,7 +784,7 @@ storage:
 class PickHTTPServerTests(unittest.SynchronousTestCase):
     """Tests for ``_pick_a_http_server``."""
 
-    def pick_result(self, url_to_results: dict[DecodedURL, tuple[float, Union[Exception, Any]]]) -> Optional[DecodedURL]:
+    def pick_result(self, url_to_results: dict[DecodedURL, tuple[float, Union[Exception, Any]]]) -> Deferred[DecodedURL]:
         """
         Given mapping of URLs to (delay, result), return the URL of the
         first selected server, or None.
@@ -803,7 +805,7 @@ class PickHTTPServerTests(unittest.SynchronousTestCase):
         d = _pick_a_http_server(clock, list(url_to_results.keys()), request)
         for i in range(100):
             clock.advance(0.1)
-        return self.successResultOf(d)
+        return d
 
     def test_first_successful_connect_is_picked(self):
         """
@@ -817,16 +819,21 @@ class PickHTTPServerTests(unittest.SynchronousTestCase):
             earliest_url: (1, None),
             bad_url: (0.5, RuntimeError()),
         })
-        self.assertEqual(result, earliest_url)
+        self.assertEqual(self.successResultOf(result), earliest_url)
 
-    def test_failures_are_turned_into_none(self):
+    def test_failures_include_all_reasons(self):
         """
-        If the requests all fail, ``_pick_a_http_server`` returns ``None``.
+        If all the requests fail, ``_pick_a_http_server`` raises a
+        ``allmydata.util.deferredutil.MultiFailure``.
         """
         eventually_good_url = DecodedURL.from_text("http://good")
         bad_url = DecodedURL.from_text("http://bad")
+        exception1 = RuntimeError()
+        exception2 = ZeroDivisionError()
         result = self.pick_result({
-            eventually_good_url: (1, ZeroDivisionError()),
-            bad_url: (0.1, RuntimeError())
+            eventually_good_url: (1, exception1),
+            bad_url: (0.1, exception2),
         })
-        self.assertEqual(result, None)
+        exc = self.failureResultOf(result).value
+        self.assertIsInstance(exc, MultiFailure)
+        self.assertEqual({f.value for f in exc.failures}, {exception2, exception1})