Some progress towards passing test_rref.

src/allmydata/storage/http_client.py

@@ -20,7 +20,7 @@ from twisted.web.http_headers import Headers
 from twisted.web import http
 from twisted.web.iweb import IPolicyForHTTPS
 from twisted.internet.defer import inlineCallbacks, returnValue, fail, Deferred, succeed
-from twisted.internet.interfaces import IOpenSSLClientConnectionCreator
+from twisted.internet.interfaces import IOpenSSLClientConnectionCreator, IReactorTime
 from twisted.internet.ssl import CertificateOptions
 from twisted.web.client import Agent, HTTPConnectionPool
 from zope.interface import implementer
@@ -282,15 +282,32 @@ class StorageClient(object):
     Low-level HTTP client that talks to the HTTP storage server.
     """
 
+    # If True, we're doing unit testing.
+    TEST_MODE = False
+
+    @classmethod
+    def start_test_mode(cls):
+        """Switch to testing mode.
+
+        In testing mode we disable persistent HTTP queries and have shorter
+        timeouts, to make certain tests work, but don't change the actual
+        semantic work being done—given a fast server, everything would work the
+        same.
+        """
+        cls.TEST_MODE = True
+
     # The URL is a HTTPS URL ("https://...").  To construct from a NURL, use
     # ``StorageClient.from_nurl()``.
     _base_url: DecodedURL
     _swissnum: bytes
     _treq: Union[treq, StubTreq, HTTPClient] = field(eq=False)
+    _clock: IReactorTime
 
     @classmethod
     def from_nurl(
-        cls, nurl: DecodedURL, reactor, persistent: bool = True
+        cls,
+        nurl: DecodedURL,
+        reactor,
     ) -> StorageClient:
         """
         Create a ``StorageClient`` for the given NURL.
@@ -302,16 +319,23 @@ class StorageClient(object):
         swissnum = nurl.path[0].encode("ascii")
         certificate_hash = nurl.user.encode("ascii")
 
+        if cls.TEST_MODE:
+            pool = HTTPConnectionPool(reactor, persistent=False)
+            pool.retryAutomatically = False
+            pool.maxPersistentPerHost = 0
+        else:
+            pool = HTTPConnectionPool(reactor)
+
         treq_client = HTTPClient(
             Agent(
                 reactor,
                 _StorageClientHTTPSPolicy(expected_spki_hash=certificate_hash),
-                pool=HTTPConnectionPool(reactor, persistent=persistent),
+                pool=pool,
             )
         )
 
         https_url = DecodedURL().replace(scheme="https", host=nurl.host, port=nurl.port)
-        return cls(https_url, swissnum, treq_client)
+        return cls(https_url, swissnum, treq_client, reactor)
 
     def relative_url(self, path):
         """Get a URL relative to the base URL."""
@@ -376,7 +400,14 @@ class StorageClient(object):
             kwargs["data"] = dumps(message_to_serialize)
             headers.addRawHeader("Content-Type", CBOR_MIME_TYPE)
 
-        return self._treq.request(method, url, headers=headers, **kwargs)
+        result = self._treq.request(method, url, headers=headers, **kwargs)
+
+        # If we're in test mode, we want an aggressive timeout, e.g. for
+        # test_rref in test_system.py.
+        if self.TEST_MODE:
+            result.addTimeout(1, self._clock)
+
+        return result
 
 
 @define(hash=True)
@@ -384,7 +415,8 @@ class StorageClientGeneral(object):
     """
     High-level HTTP APIs that aren't immutable- or mutable-specific.
     """
-    _client : StorageClient
+
+    _client: StorageClient
 
     @inlineCallbacks
     def get_version(self):

src/allmydata/storage_client.py

@@ -951,14 +951,18 @@ class HTTPNativeStorageServer(service.MultiService):
         self.announcement = announcement
         self._on_status_changed = ObserverList()
         furl = announcement["anonymous-storage-FURL"].encode("utf-8")
-        self._nickname, self._permutation_seed, self._tubid, self._short_description, self._long_description = _parse_announcement(server_id, furl, announcement)
+        (
+            self._nickname,
+            self._permutation_seed,
+            self._tubid,
+            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
         nurl = DecodedURL.from_text(announcement["anonymous-storage-NURLs"][0])
-        # Tests don't want persistent HTTPS pool, since that leaves a dirty
-        # reactor. As a reasonable hack, disabling persistent connnections for
-        # localhost allows us to have passing tests while not reducing
-        # performance for real-world usage.
         self._istorage_server = _HTTPStorageServer.from_http_client(
-            StorageClient.from_nurl(nurl, reactor, nurl.host not in ("localhost", "127.0.0.1"))
+            StorageClient.from_nurl(nurl, reactor)
         )
 
         self._connection_status = connection_status.ConnectionStatus.unstarted()

src/allmydata/test/common_system.py

@@ -28,6 +28,7 @@ from foolscap.api import flushEventualQueue
 from allmydata import client
 from allmydata.introducer.server import create_introducer
 from allmydata.util import fileutil, log, pollmixin
+from allmydata.storage import http_client
 
 from twisted.python.filepath import (
     FilePath,
@@ -645,6 +646,7 @@ def _render_section_values(values):
 class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin):
 
     def setUp(self):
+        http_client.StorageClient.start_test_mode()
         self.port_assigner = SameProcessStreamEndpointAssigner()
         self.port_assigner.setUp()
         self.addCleanup(self.port_assigner.tearDown)

src/allmydata/test/test_storage_http.py

@@ -291,6 +291,7 @@ class CustomHTTPServerTests(SyncTestCase):
 
     def setUp(self):
         super(CustomHTTPServerTests, self).setUp()
+        StorageClient.start_test_mode()
         # Could be a fixture, but will only be used in this test class so not
         # going to bother:
         self._http_server = TestApp()
@@ -298,6 +299,7 @@ class CustomHTTPServerTests(SyncTestCase):
             DecodedURL.from_text("http://127.0.0.1"),
             SWISSNUM_FOR_TEST,
             treq=StubTreq(self._http_server._app.resource()),
+            clock=Clock()
         )
 
     def test_authorization_enforcement(self):
@@ -375,6 +377,7 @@ class HttpTestFixture(Fixture):
     """
 
     def _setUp(self):
+        StorageClient.start_test_mode()
         self.clock = Clock()
         self.tempdir = self.useFixture(TempDir())
         # The global Cooperator used by Twisted (a) used by pull producers in
@@ -396,6 +399,7 @@ class HttpTestFixture(Fixture):
             DecodedURL.from_text("http://127.0.0.1"),
             SWISSNUM_FOR_TEST,
             treq=self.treq,
+            clock=self.clock,
         )
 
     def result_of_with_flush(self, d):
@@ -480,6 +484,7 @@ class GenericHTTPAPITests(SyncTestCase):
                 DecodedURL.from_text("http://127.0.0.1"),
                 b"something wrong",
                 treq=StubTreq(self.http.http_server.get_resource()),
+                clock=self.http.clock,
             )
         )
         with assert_fails_with_http_code(self, http.UNAUTHORIZED):

src/allmydata/test/test_system.py

@@ -1796,6 +1796,15 @@ class SystemTest(SystemTestMixin, RunBinTahoeMixin, unittest.TestCase):
 class Connections(SystemTestMixin, unittest.TestCase):
 
     def test_rref(self):
+        # The way the listening port is created is via
+        # SameProcessStreamEndpointAssigner (allmydata.test.common), which then
+        # makes an endpoint string parsed by AdoptedServerPort. The latter does
+        # dup(fd), which results in the filedescriptor staying alive _until the
+        # test ends_. That means that when we disown the service, we still have
+        # the listening port there on the OS level! Just the resulting
+        # connections aren't handled. So this test relies on aggressive
+        # timeouts in the HTTP client and presumably some equivalent in
+        # Foolscap, since connection refused does _not_ happen.
         self.basedir = "system/Connections/rref"
         d = self.set_up_nodes(2)
         def _start(ign):