Make cancellation more likely to happen.

src/allmydata/storage_client.py

@@ -47,7 +47,7 @@ from zope.interface import (
 )
 from twisted.python.failure import Failure
 from twisted.web import http
-from twisted.internet.task import LoopingCall, deferLater
+from twisted.internet.task import LoopingCall
 from twisted.internet import defer, reactor
 from twisted.application import service
 from twisted.plugin import (
@@ -935,20 +935,35 @@ class NativeStorageServer(service.MultiService):
         self._reconnector.reset()
 
 
-async def _pick_a_http_server(
+def _pick_a_http_server(
         reactor,
         nurls: list[DecodedURL],
         request: Callable[[Any, DecodedURL], defer.Deferred[Any]]
-) -> DecodedURL:
+) -> defer.Deferred[Optional[DecodedURL]]:
-    """Pick the first server we successfully send a request to."""
+    """Pick the first server we successfully send a request to.
-    while True:
+
-        result : defer.Deferred[Optional[DecodedURL]] = defer.Deferred()
+    Fires with ``None`` if no server was found, or with the ``DecodedURL`` of
+    the first successfully-connected server.
+    """
+
+    to_cancel : list[defer.Deferred] = []
+
+    def cancel(result: Optional[defer.Deferred]):
+        for d in to_cancel:
+            if not d.called:
+                d.cancel()
+        if result is not None:
+            result.errback(defer.CancelledError())
+
+    result : defer.Deferred[Optional[DecodedURL]] = defer.Deferred(canceller=cancel)
 
     def succeeded(nurl: DecodedURL, result=result):
         # Only need the first successful NURL:
         if result.called:
             return
         result.callback(nurl)
+        # No point in continuing other requests if we're connected:
+        cancel(None)
 
     def failed(failure, failures=[], result=result):
         # Logging errors breaks a bunch of tests, and it's not a _bug_ to
@@ -961,16 +976,11 @@ async def _pick_a_http_server(
             result.callback(None)
 
     for index, nurl in enumerate(nurls):
-            request(reactor, nurl).addCallback(
+        d = request(reactor, nurl)
-                lambda _, nurl=nurl: nurl).addCallbacks(succeeded, failed)
+        to_cancel.append(d)
+        d.addCallback(lambda _, nurl=nurl: nurl).addCallbacks(succeeded, failed)
 
-        first_nurl = await result
+    return result
-        if first_nurl is None:
-            # Failed to connect to any of the NURLs, try again in a few
-            # seconds:
-            await deferLater(reactor, 5, lambda: None)
-        else:
-            return first_nurl
 
 
 @implementer(IServer)
@@ -1117,7 +1127,21 @@ class HTTPNativeStorageServer(service.MultiService):
                     StorageClient.from_nurl(nurl, reactor)
                 ).get_version()
 
-            nurl = await _pick_a_http_server(reactor, self._nurls, request)
+            # LoopingCall.stop() doesn't cancel Deferreds, unfortunately:
+            # https://github.com/twisted/twisted/issues/11814 Thus we want
+            # store the Deferred so it gets cancelled.
+            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)
             )

src/allmydata/test/test_storage_client.py

@@ -83,7 +83,6 @@ 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,
@@ -741,7 +740,7 @@ storage:
 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]:
+    def loop_until_result(self, url_to_results: dict[DecodedURL, list[tuple[float, Union[Exception, Any]]]]) -> tuple[int, DecodedURL]:
         """
         Given mapping of URLs to list of (delay, result), return the URL of the
         first selected server.
@@ -759,12 +758,15 @@ class PickHTTPServerTests(unittest.SynchronousTestCase):
             reactor.callLater(delay, add_result_value)
             return result
 
-        d = async_to_deferred(_pick_a_http_server)(
+        iterations = 0
-            clock, list(url_to_results.keys()), request
+        while True:
-        )
+            iterations += 1
-        for i in range(1000):
+            d = _pick_a_http_server(clock, list(url_to_results.keys()), request)
+            for i in range(100):
                 clock.advance(0.1)
-        return d
+            result = self.successResultOf(d)
+            if result is not None:
+                return iterations, result
 
     def test_first_successful_connect_is_picked(self):
         """
@@ -772,11 +774,12 @@ class PickHTTPServerTests(unittest.SynchronousTestCase):
         """
         earliest_url = DecodedURL.from_text("http://a")
         latest_url = DecodedURL.from_text("http://b")
-        d = self.loop_until_result({
+        iterations, result = self.loop_until_result({
             latest_url: [(2, None)],
             earliest_url: [(1, None)]
         })
-        self.assertEqual(self.successResultOf(d), earliest_url)
+        self.assertEqual(iterations, 1)
+        self.assertEqual(result, earliest_url)
 
     def test_failures_are_retried(self):
         """
@@ -785,10 +788,11 @@ class PickHTTPServerTests(unittest.SynchronousTestCase):
         """
         eventually_good_url = DecodedURL.from_text("http://good")
         bad_url = DecodedURL.from_text("http://bad")
-        d = self.loop_until_result({
+        iterations, result = 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.assertEqual(self.successResultOf(d), eventually_good_url)
+        self.assertEqual(iterations, 3)
+        self.assertEqual(result, eventually_good_url)