From 0d23237b11aea61241a75e4d19c6df394b9de0b2 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 13 Oct 2022 13:44:49 -0400 Subject: [PATCH] Some progress towards passing test_rref. --- src/allmydata/storage/http_client.py | 44 +++++++++++++++++++++---- src/allmydata/storage_client.py | 16 +++++---- src/allmydata/test/common_system.py | 2 ++ src/allmydata/test/test_storage_http.py | 5 +++ src/allmydata/test/test_system.py | 9 +++++ 5 files changed, 64 insertions(+), 12 deletions(-) diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index 1fe9a99fd..2589d4e41 100644 --- a/src/allmydata/storage/http_client.py +++ b/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): diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 51b1eabca..d492ee4cf 100644 --- a/src/allmydata/storage_client.py +++ b/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() diff --git a/src/allmydata/test/common_system.py b/src/allmydata/test/common_system.py index 75379bbf3..ef4b65529 100644 --- a/src/allmydata/test/common_system.py +++ b/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) diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py index 4a912cf6c..819c94f83 100644 --- a/src/allmydata/test/test_storage_http.py +++ b/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): diff --git a/src/allmydata/test/test_system.py b/src/allmydata/test/test_system.py index d859a0e00..d94b4d163 100644 --- a/src/allmydata/test/test_system.py +++ b/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):