From 709f139c85e00f452ca5dacb308fd3494eb1be4a Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 11 Aug 2022 15:51:30 -0400 Subject: [PATCH 01/47] Start refactoring to enable HTTP storage client. --- src/allmydata/storage_client.py | 183 ++++++++++++++++++++++++++------ 1 file changed, 151 insertions(+), 32 deletions(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index c63bfccff..a058ae828 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -30,6 +30,8 @@ Ported to Python 3. # # 6: implement other sorts of IStorageClient classes: S3, etc +from __future__ import annotations + from six import ensure_text from typing import Union import re, time, hashlib @@ -523,6 +525,45 @@ class IFoolscapStorageServer(Interface): """ +def _parse_announcement(server_id: bytes, furl: bytes, ann: dict) -> tuple[str, bytes, bytes, bytes, bytes]: + """ + Parse the furl and announcement, return: + + (nickname, permutation_seed, tubid, short_description, long_description) + """ + m = re.match(br'pb://(\w+)@', furl) + assert m, furl + tubid_s = m.group(1).lower() + tubid = base32.a2b(tubid_s) + if "permutation-seed-base32" in ann: + seed = ann["permutation-seed-base32"] + if isinstance(seed, str): + seed = seed.encode("utf-8") + ps = base32.a2b(seed) + elif re.search(br'^v0-[0-9a-zA-Z]{52}$', server_id): + ps = base32.a2b(server_id[3:]) + else: + log.msg("unable to parse serverid '%(server_id)s as pubkey, " + "hashing it to get permutation-seed, " + "may not converge with other clients", + server_id=server_id, + facility="tahoe.storage_broker", + level=log.UNUSUAL, umid="qu86tw") + ps = hashlib.sha256(server_id).digest() + permutation_seed = ps + + assert server_id + long_description = server_id + if server_id.startswith(b"v0-"): + # remove v0- prefix from abbreviated name + short_description = server_id[3:3+8] + else: + short_description = server_id[:8] + nickname = ann.get("nickname", "") + + return (nickname, permutation_seed, tubid, short_description, long_description) + + @implementer(IFoolscapStorageServer) @attr.s(frozen=True) class _FoolscapStorage(object): @@ -566,43 +607,13 @@ class _FoolscapStorage(object): The furl will be a Unicode string on Python 3; on Python 2 it will be either a native (bytes) string or a Unicode string. """ - furl = furl.encode("utf-8") - m = re.match(br'pb://(\w+)@', furl) - assert m, furl - tubid_s = m.group(1).lower() - tubid = base32.a2b(tubid_s) - if "permutation-seed-base32" in ann: - seed = ann["permutation-seed-base32"] - if isinstance(seed, str): - seed = seed.encode("utf-8") - ps = base32.a2b(seed) - elif re.search(br'^v0-[0-9a-zA-Z]{52}$', server_id): - ps = base32.a2b(server_id[3:]) - else: - log.msg("unable to parse serverid '%(server_id)s as pubkey, " - "hashing it to get permutation-seed, " - "may not converge with other clients", - server_id=server_id, - facility="tahoe.storage_broker", - level=log.UNUSUAL, umid="qu86tw") - ps = hashlib.sha256(server_id).digest() - permutation_seed = ps - - assert server_id - long_description = server_id - if server_id.startswith(b"v0-"): - # remove v0- prefix from abbreviated name - short_description = server_id[3:3+8] - else: - short_description = server_id[:8] - nickname = ann.get("nickname", "") - + (nickname, permutation_seed, tubid, short_description, long_description) = _parse_announcement(server_id, furl.encode("utf-8"), ann) return cls( nickname=nickname, permutation_seed=permutation_seed, tubid=tubid, storage_server=storage_server, - furl=furl, + furl=furl.encode("utf-8"), short_description=short_description, long_description=long_description, ) @@ -910,6 +921,114 @@ class NativeStorageServer(service.MultiService): # used when the broker wants us to hurry up self._reconnector.reset() + +@implementer(IServer) +class HTTPNativeStorageServer(service.MultiService): + """ + Like ``NativeStorageServer``, but for HTTP clients. + + The notion of being "connected" is less meaningful for HTTP; we just poll + occasionally, and if we've succeeded at last poll, we assume we're + "connected". + """ + + def __init__(self, server_id: bytes, announcement): + service.MultiService.__init__(self) + assert isinstance(server_id, bytes) + self._server_id = server_id + 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) + + def get_permutation_seed(self): + return self._permutation_seed + + def get_name(self): # keep methodname short + return self._name + + def get_longname(self): + return self._longname + + def get_tubid(self): + return self._tubid + + def get_lease_seed(self): + return self._lease_seed + + def get_foolscap_write_enabler_seed(self): + return self._tubid + + def get_nickname(self): + return self._nickname + + def on_status_changed(self, status_changed): + """ + :param status_changed: a callable taking a single arg (the + NativeStorageServer) that is notified when we become connected + """ + return self._on_status_changed.subscribe(status_changed) + + # Special methods used by copy.copy() and copy.deepcopy(). When those are + # used in allmydata.immutable.filenode to copy CheckResults during + # repair, we want it to treat the IServer instances as singletons, and + # not attempt to duplicate them.. + def __copy__(self): + return self + + def __deepcopy__(self, memodict): + return self + + def __repr__(self): + return "" % self.get_name() + + def get_serverid(self): + return self._server_id + + def get_version(self): + pass + + def get_announcement(self): + return self.announcement + + def get_connection_status(self): + pass + + def is_connected(self): + pass + + def get_available_space(self): + # TODO refactor into shared utility with NativeStorageServer + version = self.get_version() + if version is None: + return None + protocol_v1_version = version.get(b'http://allmydata.org/tahoe/protocols/storage/v1', BytesKeyDict()) + available_space = protocol_v1_version.get(b'available-space') + if available_space is None: + available_space = protocol_v1_version.get(b'maximum-immutable-share-size', None) + return available_space + + def start_connecting(self, trigger_cb): + pass + + def get_rref(self): + # TODO UH + pass + + def get_storage_server(self): + """ + See ``IServer.get_storage_server``. + """ + + def stop_connecting(self): + # used when this descriptor has been superceded by another + pass + + def try_to_connect(self): + # used when the broker wants us to hurry up + pass + + class UnknownServerTypeError(Exception): pass From c3e41588130e9d2c9d65965a9ea06d8f3503bd52 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 11 Aug 2022 15:55:14 -0400 Subject: [PATCH 02/47] Remove duplication. --- src/allmydata/storage_client.py | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index a058ae828..e64f63413 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -695,6 +695,16 @@ def _storage_from_foolscap_plugin(node_config, config, announcement, get_rref): raise AnnouncementNotMatched() +def _available_space_from_version(version): + if version is None: + return None + protocol_v1_version = version.get(b'http://allmydata.org/tahoe/protocols/storage/v1', BytesKeyDict()) + available_space = protocol_v1_version.get(b'available-space') + if available_space is None: + available_space = protocol_v1_version.get(b'maximum-immutable-share-size', None) + return available_space + + @implementer(IServer) class NativeStorageServer(service.MultiService): """I hold information about a storage server that we want to connect to. @@ -853,13 +863,7 @@ class NativeStorageServer(service.MultiService): def get_available_space(self): version = self.get_version() - if version is None: - return None - protocol_v1_version = version.get(b'http://allmydata.org/tahoe/protocols/storage/v1', BytesKeyDict()) - available_space = protocol_v1_version.get(b'available-space') - if available_space is None: - available_space = protocol_v1_version.get(b'maximum-immutable-share-size', None) - return available_space + return _available_space_from_version(version) def start_connecting(self, trigger_cb): self._tub = self._tub_maker(self._handler_overrides) @@ -998,15 +1002,8 @@ class HTTPNativeStorageServer(service.MultiService): pass def get_available_space(self): - # TODO refactor into shared utility with NativeStorageServer version = self.get_version() - if version is None: - return None - protocol_v1_version = version.get(b'http://allmydata.org/tahoe/protocols/storage/v1', BytesKeyDict()) - available_space = protocol_v1_version.get(b'available-space') - if available_space is None: - available_space = protocol_v1_version.get(b'maximum-immutable-share-size', None) - return available_space + return _available_space_from_version(version) def start_connecting(self, trigger_cb): pass From c3b159a3fd98e63bcc0641c4c2a5dffe5e795a15 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 11 Aug 2022 16:12:57 -0400 Subject: [PATCH 03/47] Continue simplified sketch of HTTPNativeStorageServer. --- src/allmydata/storage_client.py | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index e64f63413..3bcd8e6db 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -45,7 +45,7 @@ from zope.interface import ( implementer, ) from twisted.web import http -from twisted.internet import defer +from twisted.internet import defer, reactor from twisted.application import service from twisted.plugin import ( getPlugins, @@ -934,6 +934,9 @@ class HTTPNativeStorageServer(service.MultiService): The notion of being "connected" is less meaningful for HTTP; we just poll occasionally, and if we've succeeded at last poll, we assume we're "connected". + + TODO as first pass, just to get the proof-of-concept going, we will just + assume we're always connected after an initial successful HTTP request. """ def __init__(self, server_id: bytes, announcement): @@ -944,6 +947,13 @@ class HTTPNativeStorageServer(service.MultiService): 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._istorage_server = _HTTPStorageServer.from_http_client( + StorageClient.from_nurl( + announcement["anonymous-storage-NURLs"][0], reactor + ) + ) + self._connection_status = connection_status.ConnectionStatus.unstarted() + self._version = None def get_permutation_seed(self): return self._permutation_seed @@ -984,29 +994,33 @@ class HTTPNativeStorageServer(service.MultiService): return self def __repr__(self): - return "" % self.get_name() + return "" % self.get_name() def get_serverid(self): return self._server_id def get_version(self): - pass + return self._version def get_announcement(self): return self.announcement def get_connection_status(self): - pass + return self._connection_status def is_connected(self): - pass + return self._connection_status.connected def get_available_space(self): version = self.get_version() return _available_space_from_version(version) def start_connecting(self, trigger_cb): - pass + self._istorage_server.get_version().addCallback(self._got_version) + + def _got_version(self, version): + self._version = version + self._connection_status = connection_status.ConnectionStatus(True, "connected", [], time.time(), time.time()) def get_rref(self): # TODO UH @@ -1016,13 +1030,15 @@ class HTTPNativeStorageServer(service.MultiService): """ See ``IServer.get_storage_server``. """ + if self.is_connected(): + return self._istorage_server + else: + return None def stop_connecting(self): - # used when this descriptor has been superceded by another pass def try_to_connect(self): - # used when the broker wants us to hurry up pass From 94be227aaaf7adfefc3457dbc7556b10c7b5f3c4 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 11 Aug 2022 16:15:21 -0400 Subject: [PATCH 04/47] Hopefully don't actually need that. --- src/allmydata/storage_client.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 3bcd8e6db..62cc047f2 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -1022,10 +1022,6 @@ class HTTPNativeStorageServer(service.MultiService): self._version = version self._connection_status = connection_status.ConnectionStatus(True, "connected", [], time.time(), time.time()) - def get_rref(self): - # TODO UH - pass - def get_storage_server(self): """ See ``IServer.get_storage_server``. From 9ad4e844e86302682dfd38e82b7e262231c21ad9 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 11 Aug 2022 16:16:17 -0400 Subject: [PATCH 05/47] Do status change notification. --- src/allmydata/storage_client.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 62cc047f2..254179559 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -937,6 +937,7 @@ class HTTPNativeStorageServer(service.MultiService): TODO as first pass, just to get the proof-of-concept going, we will just assume we're always connected after an initial successful HTTP request. + Might do polling as follow-up ticket, in which case add link to that here. """ def __init__(self, server_id: bytes, announcement): @@ -1021,6 +1022,7 @@ class HTTPNativeStorageServer(service.MultiService): def _got_version(self, version): self._version = version self._connection_status = connection_status.ConnectionStatus(True, "connected", [], time.time(), time.time()) + self._on_status_changed.notify(self) def get_storage_server(self): """ From f671fb04a18c5a3f20437eac3424db1f97fc5df4 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 11 Aug 2022 16:24:33 -0400 Subject: [PATCH 06/47] A lot closer to working end-to-end. --- src/allmydata/client.py | 6 +++--- src/allmydata/storage_client.py | 9 ++++++++- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/allmydata/client.py b/src/allmydata/client.py index 9938ec076..769554b3d 100644 --- a/src/allmydata/client.py +++ b/src/allmydata/client.py @@ -825,9 +825,9 @@ class _Client(node.Node, pollmixin.PollMixin): furl_file = self.config.get_private_path("storage.furl").encode(get_filesystem_encoding()) furl = self.tub.registerReference(FoolscapStorageServer(ss), furlFile=furl_file) (_, _, swissnum) = furl.rpartition("/") - self.storage_nurls.update( - self.tub.negotiationClass.add_storage_server(ss, swissnum.encode("ascii")) - ) + nurls = self.tub.negotiationClass.add_storage_server(ss, swissnum.encode("ascii")) + self.storage_nurls.update(nurls) + announcement["anonymous-storage-NURLs"] = [n.to_text() for n in nurls] announcement["anonymous-storage-FURL"] = furl enabled_storage_servers = self._enable_storage_servers( diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 254179559..ec03393a1 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -39,6 +39,7 @@ from os import urandom from configparser import NoSectionError import attr +from hyperlink import DecodedURL from zope.interface import ( Attribute, Interface, @@ -264,6 +265,12 @@ class StorageFarmBroker(service.MultiService): by the given announcement. """ assert isinstance(server_id, bytes) + # TODO use constant + if "anonymous-storage-NURLs" in server["ann"]: + print("HTTTTTTTPPPPPPPPPPPPPPPPPPPP") + s = HTTPNativeStorageServer(server_id, server["ann"]) + s.on_status_changed(lambda _: self._got_connection()) + return s handler_overrides = server.get("connections", {}) s = NativeStorageServer( server_id, @@ -950,7 +957,7 @@ class HTTPNativeStorageServer(service.MultiService): self._nickname, self._permutation_seed, self._tubid, self._short_description, self._long_description = _parse_announcement(server_id, furl, announcement) self._istorage_server = _HTTPStorageServer.from_http_client( StorageClient.from_nurl( - announcement["anonymous-storage-NURLs"][0], reactor + DecodedURL.from_text(announcement["anonymous-storage-NURLs"][0]), reactor ) ) self._connection_status = connection_status.ConnectionStatus.unstarted() From 09d778c2cfb4f888831835903daf6d77205ff5c7 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 12 Aug 2022 11:13:09 -0400 Subject: [PATCH 07/47] Allow nodes to disable the HTTPS storage protocol. --- src/allmydata/client.py | 7 ++++--- src/allmydata/node.py | 5 ++++- src/allmydata/storage_client.py | 4 ++-- src/allmydata/test/common_system.py | 20 +++++++++++++------- 4 files changed, 23 insertions(+), 13 deletions(-) diff --git a/src/allmydata/client.py b/src/allmydata/client.py index 769554b3d..d9fc20e92 100644 --- a/src/allmydata/client.py +++ b/src/allmydata/client.py @@ -825,9 +825,10 @@ class _Client(node.Node, pollmixin.PollMixin): furl_file = self.config.get_private_path("storage.furl").encode(get_filesystem_encoding()) furl = self.tub.registerReference(FoolscapStorageServer(ss), furlFile=furl_file) (_, _, swissnum) = furl.rpartition("/") - nurls = self.tub.negotiationClass.add_storage_server(ss, swissnum.encode("ascii")) - self.storage_nurls.update(nurls) - announcement["anonymous-storage-NURLs"] = [n.to_text() for n in nurls] + if hasattr(self.tub.negotiationClass, "add_storage_server"): + nurls = self.tub.negotiationClass.add_storage_server(ss, swissnum.encode("ascii")) + self.storage_nurls.update(nurls) + announcement["anonymous-storage-NURLs"] = [n.to_text() for n in nurls] announcement["anonymous-storage-FURL"] = furl enabled_storage_servers = self._enable_storage_servers( diff --git a/src/allmydata/node.py b/src/allmydata/node.py index 597221e9b..0ad68f2b7 100644 --- a/src/allmydata/node.py +++ b/src/allmydata/node.py @@ -64,6 +64,7 @@ def _common_valid_config(): "tcp", ), "node": ( + "force_foolscap", "log_gatherer.furl", "nickname", "reveal-ip-address", @@ -709,7 +710,6 @@ def create_tub(tub_options, default_connection_handlers, foolscap_connection_han the new Tub via `Tub.setOption` """ tub = Tub(**kwargs) - support_foolscap_and_https(tub) for (name, value) in list(tub_options.items()): tub.setOption(name, value) handlers = default_connection_handlers.copy() @@ -907,6 +907,9 @@ def create_main_tub(config, tub_options, handler_overrides=handler_overrides, certFile=certfile, ) + if not config.get_config("node", "force_foolscap", False): + support_foolscap_and_https(tub) + if portlocation is None: log.msg("Tub is not listening") else: diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index ec03393a1..3c2c7a1b8 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -102,8 +102,8 @@ class StorageClientConfig(object): :ivar preferred_peers: An iterable of the server-ids (``bytes``) of the storage servers where share placement is preferred, in order of - decreasing preference. See the *[client]peers.preferred* - documentation for details. + decreasing preference. See the *[client]peers.preferred* documentation + for details. :ivar dict[unicode, dict[unicode, unicode]] storage_plugins: A mapping from names of ``IFoolscapStoragePlugin`` configured in *tahoe.cfg* to the diff --git a/src/allmydata/test/common_system.py b/src/allmydata/test/common_system.py index 9851d2b91..75379bbf3 100644 --- a/src/allmydata/test/common_system.py +++ b/src/allmydata/test/common_system.py @@ -698,7 +698,7 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): return f.read().strip() @inlineCallbacks - def set_up_nodes(self, NUMCLIENTS=5): + def set_up_nodes(self, NUMCLIENTS=5, force_foolscap=False): """ Create an introducer and ``NUMCLIENTS`` client nodes pointed at it. All of the nodes are running in this process. @@ -711,6 +711,9 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): :param int NUMCLIENTS: The number of client nodes to create. + :param bool force_foolscap: Force clients to use Foolscap instead of e.g. + HTTPS when available. + :return: A ``Deferred`` that fires when the nodes have connected to each other. """ @@ -719,16 +722,16 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): self.introducer = yield self._create_introducer() self.add_service(self.introducer) self.introweb_url = self._get_introducer_web() - yield self._set_up_client_nodes() + yield self._set_up_client_nodes(force_foolscap) @inlineCallbacks - def _set_up_client_nodes(self): + def _set_up_client_nodes(self, force_foolscap): q = self.introducer self.introducer_furl = q.introducer_url self.clients = [] basedirs = [] for i in range(self.numclients): - basedirs.append((yield self._set_up_client_node(i))) + basedirs.append((yield self._set_up_client_node(i, force_foolscap))) # start clients[0], wait for it's tub to be ready (at which point it # will have registered the helper furl). @@ -761,7 +764,7 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): # and the helper-using webport self.helper_webish_url = self.clients[3].getServiceNamed("webish").getURL() - def _generate_config(self, which, basedir): + def _generate_config(self, which, basedir, force_foolscap=False): config = {} allclients = set(range(self.numclients)) @@ -787,6 +790,9 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): if which in feature_matrix.get((section, feature), {which}): config.setdefault(section, {})[feature] = value + if force_foolscap: + config.setdefault("node", {})["force_foolscap"] = force_foolscap + setnode = partial(setconf, config, which, "node") sethelper = partial(setconf, config, which, "helper") @@ -811,14 +817,14 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): return _render_config(config) - def _set_up_client_node(self, which): + def _set_up_client_node(self, which, force_foolscap): basedir = self.getdir("client%d" % (which,)) fileutil.make_dirs(os.path.join(basedir, "private")) if len(SYSTEM_TEST_CERTS) > (which + 1): f = open(os.path.join(basedir, "private", "node.pem"), "w") f.write(SYSTEM_TEST_CERTS[which + 1]) f.close() - config = self._generate_config(which, basedir) + config = self._generate_config(which, basedir, force_foolscap) fileutil.write(os.path.join(basedir, 'tahoe.cfg'), config) return basedir From e8609ac2df01038a7c51952149aaf4566b24e271 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 12 Aug 2022 11:24:41 -0400 Subject: [PATCH 08/47] test_istorageserver passes with both Foolscap and HTTP again. --- src/allmydata/storage_client.py | 14 +++++----- src/allmydata/test/test_istorageserver.py | 33 ++++++++++++----------- 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 3c2c7a1b8..e2a48e521 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -265,9 +265,8 @@ class StorageFarmBroker(service.MultiService): by the given announcement. """ assert isinstance(server_id, bytes) - # TODO use constant - if "anonymous-storage-NURLs" in server["ann"]: - print("HTTTTTTTPPPPPPPPPPPPPPPPPPPP") + # TODO use constant for anonymous-storage-NURLs + if len(server["ann"].get("anonymous-storage-NURLs", [])) > 0: s = HTTPNativeStorageServer(server_id, server["ann"]) s.on_status_changed(lambda _: self._got_connection()) return s @@ -955,10 +954,13 @@ class HTTPNativeStorageServer(service.MultiService): 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) + 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( - DecodedURL.from_text(announcement["anonymous-storage-NURLs"][0]), reactor - ) + StorageClient.from_nurl(nurl, reactor, nurl.host not in ("localhost", "127.0.0.1")) ) self._connection_status = connection_status.ConnectionStatus.unstarted() self._version = None diff --git a/src/allmydata/test/test_istorageserver.py b/src/allmydata/test/test_istorageserver.py index 3328ea598..81025d779 100644 --- a/src/allmydata/test/test_istorageserver.py +++ b/src/allmydata/test/test_istorageserver.py @@ -17,7 +17,6 @@ from unittest import SkipTest from twisted.internet.defer import inlineCallbacks, returnValue, succeed from twisted.internet.task import Clock -from twisted.internet import reactor from foolscap.api import Referenceable, RemoteException # A better name for this would be IStorageClient... @@ -26,8 +25,10 @@ from allmydata.interfaces import IStorageServer from .common_system import SystemTestMixin from .common import AsyncTestCase from allmydata.storage.server import StorageServer # not a IStorageServer!! -from allmydata.storage.http_client import StorageClient -from allmydata.storage_client import _HTTPStorageServer +from allmydata.storage_client import ( + NativeStorageServer, + HTTPNativeStorageServer, +) # Use random generator with known seed, so results are reproducible if tests @@ -1021,6 +1022,10 @@ class _SharedMixin(SystemTestMixin): """Base class for Foolscap and HTTP mixins.""" SKIP_TESTS = set() # type: Set[str] + FORCE_FOOLSCAP = False + + def _get_native_server(self): + return next(iter(self.clients[0].storage_broker.get_known_servers())) def _get_istorage_server(self): raise NotImplementedError("implement in subclass") @@ -1036,7 +1041,7 @@ class _SharedMixin(SystemTestMixin): self.basedir = "test_istorageserver/" + self.id() yield SystemTestMixin.setUp(self) - yield self.set_up_nodes(1) + yield self.set_up_nodes(1, self.FORCE_FOOLSCAP) self.server = None for s in self.clients[0].services: if isinstance(s, StorageServer): @@ -1065,11 +1070,12 @@ class _SharedMixin(SystemTestMixin): class _FoolscapMixin(_SharedMixin): """Run tests on Foolscap version of ``IStorageServer``.""" - def _get_native_server(self): - return next(iter(self.clients[0].storage_broker.get_known_servers())) + FORCE_FOOLSCAP = True def _get_istorage_server(self): - client = self._get_native_server().get_storage_server() + native_server = self._get_native_server() + assert isinstance(native_server, NativeStorageServer) + client = native_server.get_storage_server() self.assertTrue(IStorageServer.providedBy(client)) return succeed(client) @@ -1077,16 +1083,13 @@ class _FoolscapMixin(_SharedMixin): class _HTTPMixin(_SharedMixin): """Run tests on the HTTP version of ``IStorageServer``.""" + FORCE_FOOLSCAP = False + def _get_istorage_server(self): - nurl = list(self.clients[0].storage_nurls)[0] - - # Create HTTP client with non-persistent connections, so we don't leak - # state across tests: - client: IStorageServer = _HTTPStorageServer.from_http_client( - StorageClient.from_nurl(nurl, reactor, persistent=False) - ) + native_server = self._get_native_server() + assert isinstance(native_server, HTTPNativeStorageServer) + client = native_server.get_storage_server() self.assertTrue(IStorageServer.providedBy(client)) - return succeed(client) From 3fbc4d7eea3398075a6986416a585f993549cc65 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 12 Aug 2022 11:45:37 -0400 Subject: [PATCH 09/47] Let's make this a little clearer --- src/allmydata/scripts/tahoe_run.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/scripts/tahoe_run.py b/src/allmydata/scripts/tahoe_run.py index 51be32ee3..dfdc97ea5 100644 --- a/src/allmydata/scripts/tahoe_run.py +++ b/src/allmydata/scripts/tahoe_run.py @@ -179,7 +179,7 @@ class DaemonizeTheRealService(Service, HookMixin): ) ) else: - self.stderr.write("\nUnknown error\n") + self.stderr.write("\nUnknown error, here's the traceback:\n") reason.printTraceback(self.stderr) reactor.stop() From 42e818f0a702738ceae40d033fa69d43a68e5657 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 12 Aug 2022 11:47:08 -0400 Subject: [PATCH 10/47] Refer to appropriate attributes, hopefully. --- src/allmydata/storage_client.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index e2a48e521..87041ff8b 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -968,17 +968,18 @@ class HTTPNativeStorageServer(service.MultiService): def get_permutation_seed(self): return self._permutation_seed - def get_name(self): # keep methodname short - return self._name + def get_name(self): + return self._short_description def get_longname(self): - return self._longname + return self._long_description def get_tubid(self): return self._tubid def get_lease_seed(self): - return self._lease_seed + # Apparently this is what Foolscap version above does?! + return self._tubid def get_foolscap_write_enabler_seed(self): return self._tubid From c1bcfab7f80d9a1f3e7b5f2e8c39dd292daedcd9 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Mon, 15 Aug 2022 11:38:02 -0400 Subject: [PATCH 11/47] Repeatedly poll status of server. --- src/allmydata/storage_client.py | 36 +++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 87041ff8b..f9a6feb7d 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -46,6 +46,7 @@ from zope.interface import ( implementer, ) from twisted.web import http +from twisted.internet.task import LoopingCall from twisted.internet import defer, reactor from twisted.application import service from twisted.plugin import ( @@ -940,10 +941,6 @@ class HTTPNativeStorageServer(service.MultiService): The notion of being "connected" is less meaningful for HTTP; we just poll occasionally, and if we've succeeded at last poll, we assume we're "connected". - - TODO as first pass, just to get the proof-of-concept going, we will just - assume we're always connected after an initial successful HTTP request. - Might do polling as follow-up ticket, in which case add link to that here. """ def __init__(self, server_id: bytes, announcement): @@ -962,8 +959,10 @@ class HTTPNativeStorageServer(service.MultiService): self._istorage_server = _HTTPStorageServer.from_http_client( StorageClient.from_nurl(nurl, reactor, nurl.host not in ("localhost", "127.0.0.1")) ) + self._connection_status = connection_status.ConnectionStatus.unstarted() self._version = None + self._last_connect_time = None def get_permutation_seed(self): return self._permutation_seed @@ -1027,11 +1026,21 @@ class HTTPNativeStorageServer(service.MultiService): return _available_space_from_version(version) def start_connecting(self, trigger_cb): - self._istorage_server.get_version().addCallback(self._got_version) + self._lc = LoopingCall(self._connect) + self._lc.start(1, True) def _got_version(self, version): + self._last_connect_time = time.time() self._version = version - self._connection_status = connection_status.ConnectionStatus(True, "connected", [], time.time(), time.time()) + self._connection_status = connection_status.ConnectionStatus( + True, "connected", [], self._last_connect_time, self._last_connect_time + ) + self._on_status_changed.notify(self) + + def _failed_to_connect(self, reason): + self._connection_status = connection_status.ConnectionStatus( + False, f"failure: {reason}", [], self._last_connect_time, self._last_connect_time + ) self._on_status_changed.notify(self) def get_storage_server(self): @@ -1044,10 +1053,21 @@ class HTTPNativeStorageServer(service.MultiService): return None def stop_connecting(self): - pass + self._lc.stop() def try_to_connect(self): - pass + self._connect() + + def _connect(self): + return self._istorage_server.get_version().addCallbacks( + self._got_version, + self._failed_to_connect + ) + + def stopService(self): + service.MultiService.stopService(self) + self._lc.stop() + self._failed_to_connect("shut down") class UnknownServerTypeError(Exception): From 8b2884cf3a1ce0d4d17c8483202b48055646b7ed Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 4 Oct 2022 09:44:30 -0400 Subject: [PATCH 12/47] Make changes work again. --- src/allmydata/node.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/allmydata/node.py b/src/allmydata/node.py index 6747a3c77..7d33d220a 100644 --- a/src/allmydata/node.py +++ b/src/allmydata/node.py @@ -698,7 +698,7 @@ def create_connection_handlers(config, i2p_provider, tor_provider): def create_tub(tub_options, default_connection_handlers, foolscap_connection_handlers, - handler_overrides={}, **kwargs): + handler_overrides={}, force_foolscap=False, **kwargs): """ Create a Tub with the right options and handlers. It will be ephemeral unless the caller provides certFile= in kwargs @@ -708,10 +708,16 @@ def create_tub(tub_options, default_connection_handlers, foolscap_connection_han :param dict tub_options: every key-value pair in here will be set in the new Tub via `Tub.setOption` + + :param bool force_foolscap: If True, only allow Foolscap, not just HTTPS + storage protocol. """ - # We listen simulataneously for both Foolscap and HTTPS on the same port, + # We listen simultaneously for both Foolscap and HTTPS on the same port, # so we have to create a special Foolscap Tub for that to work: - tub = create_tub_with_https_support(**kwargs) + if force_foolscap: + tub = Tub(**kwargs) + else: + tub = create_tub_with_https_support(**kwargs) for (name, value) in list(tub_options.items()): tub.setOption(name, value) @@ -907,11 +913,10 @@ def create_main_tub(config, tub_options, tub_options, default_connection_handlers, foolscap_connection_handlers, + force_foolscap=config.get_config("node", "force_foolscap", False), handler_overrides=handler_overrides, certFile=certfile, ) - if not config.get_config("node", "force_foolscap", False): - support_foolscap_and_https(tub) if portlocation is None: log.msg("Tub is not listening") From fd07c092edf9e0367a0f2c6d770273a4ba1f6a52 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 4 Oct 2022 10:30:07 -0400 Subject: [PATCH 13/47] close() is called while writes are still happening. --- src/allmydata/storage_client.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index f9a6feb7d..4ab818b9c 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -1211,7 +1211,7 @@ class _HTTPBucketWriter(object): storage_index = attr.ib(type=bytes) share_number = attr.ib(type=int) upload_secret = attr.ib(type=bytes) - finished = attr.ib(type=bool, default=False) + finished = attr.ib(type=defer.Deferred[bool], factory=defer.Deferred) def abort(self): return self.client.abort_upload(self.storage_index, self.share_number, @@ -1223,14 +1223,13 @@ class _HTTPBucketWriter(object): self.storage_index, self.share_number, self.upload_secret, offset, data ) if result.finished: - self.finished = True + self.finished.callback(True) defer.returnValue(None) def close(self): - # A no-op in HTTP protocol. - if not self.finished: - return defer.fail(RuntimeError("You didn't finish writing?!")) - return defer.succeed(None) + # We're not _really_ closed until all writes have succeeded and we + # finished writing all the data. + return self.finished From 1294baa82e71e1d4cd8c63fc2c3f6e3041062505 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 4 Oct 2022 10:30:27 -0400 Subject: [PATCH 14/47] LoopingCall may already have been stopped. --- src/allmydata/storage_client.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 4ab818b9c..a7d5edb11 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -1066,7 +1066,8 @@ class HTTPNativeStorageServer(service.MultiService): def stopService(self): service.MultiService.stopService(self) - self._lc.stop() + if self._lc.running: + self._lc.stop() self._failed_to_connect("shut down") From ea1d2486115b848ec5a8409eae328792e5d2a338 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 4 Oct 2022 10:51:43 -0400 Subject: [PATCH 15/47] These objects get stored in a context where they need to be hashed, sometimes. --- src/allmydata/storage/http_client.py | 11 +++++------ src/allmydata/storage_client.py | 5 ++--- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index 16d426dda..1fe9a99fd 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -276,7 +276,7 @@ class _StorageClientHTTPSPolicy: ) -@define +@define(hash=True) class StorageClient(object): """ Low-level HTTP client that talks to the HTTP storage server. @@ -286,7 +286,7 @@ class StorageClient(object): # ``StorageClient.from_nurl()``. _base_url: DecodedURL _swissnum: bytes - _treq: Union[treq, StubTreq, HTTPClient] + _treq: Union[treq, StubTreq, HTTPClient] = field(eq=False) @classmethod def from_nurl( @@ -379,13 +379,12 @@ class StorageClient(object): return self._treq.request(method, url, headers=headers, **kwargs) +@define(hash=True) class StorageClientGeneral(object): """ High-level HTTP APIs that aren't immutable- or mutable-specific. """ - - def __init__(self, client): # type: (StorageClient) -> None - self._client = client + _client : StorageClient @inlineCallbacks def get_version(self): @@ -534,7 +533,7 @@ async def advise_corrupt_share( ) -@define +@define(hash=True) class StorageClientImmutables(object): """ APIs for interacting with immutables. diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index a7d5edb11..3b08f0b25 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -1187,7 +1187,7 @@ class _StorageServer(object): -@attr.s +@attr.s(hash=True) class _FakeRemoteReference(object): """ Emulate a Foolscap RemoteReference, calling a local object instead. @@ -1203,7 +1203,6 @@ class _FakeRemoteReference(object): raise RemoteException(e.args) -@attr.s class _HTTPBucketWriter(object): """ Emulate a ``RIBucketWriter``, but use HTTP protocol underneath. @@ -1234,7 +1233,7 @@ class _HTTPBucketWriter(object): -@attr.s +@attr.s(hash=True) class _HTTPBucketReader(object): """ Emulate a ``RIBucketReader``, but use HTTP protocol underneath. From 8190eea48924a095bf8c681fc3a7b9960d7ed839 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 4 Oct 2022 11:02:36 -0400 Subject: [PATCH 16/47] Fix bug introduced in previous commit. --- src/allmydata/storage_client.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 3b08f0b25..6d59b4f7d 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -1203,6 +1203,7 @@ class _FakeRemoteReference(object): raise RemoteException(e.args) +@attr.s class _HTTPBucketWriter(object): """ Emulate a ``RIBucketWriter``, but use HTTP protocol underneath. From 8b0ddf406e2863d0991f287032efbb203a15c8c4 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 4 Oct 2022 11:17:19 -0400 Subject: [PATCH 17/47] Make HTTP and Foolscap match in another edge case. --- src/allmydata/storage_client.py | 15 ++++++++++++-- src/allmydata/test/test_istorageserver.py | 24 +++++++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 6d59b4f7d..51b1eabca 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -45,6 +45,7 @@ from zope.interface import ( Interface, implementer, ) +from twisted.python.failure import Failure from twisted.web import http from twisted.internet.task import LoopingCall from twisted.internet import defer, reactor @@ -1233,6 +1234,16 @@ class _HTTPBucketWriter(object): return self.finished +def _ignore_404(failure: Failure) -> Union[Failure, None]: + """ + Useful for advise_corrupt_share(), since it swallows unknown share numbers + in Foolscap. + """ + if failure.check(HTTPClientException) and failure.value.code == http.NOT_FOUND: + return None + else: + return failure + @attr.s(hash=True) class _HTTPBucketReader(object): @@ -1252,7 +1263,7 @@ class _HTTPBucketReader(object): return self.client.advise_corrupt_share( self.storage_index, self.share_number, str(reason, "utf-8", errors="backslashreplace") - ) + ).addErrback(_ignore_404) # WORK IN PROGRESS, for now it doesn't actually implement whole thing. @@ -1352,7 +1363,7 @@ class _HTTPStorageServer(object): raise ValueError("Unknown share type") return client.advise_corrupt_share( storage_index, shnum, str(reason, "utf-8", errors="backslashreplace") - ) + ).addErrback(_ignore_404) @defer.inlineCallbacks def slot_readv(self, storage_index, shares, readv): diff --git a/src/allmydata/test/test_istorageserver.py b/src/allmydata/test/test_istorageserver.py index 81025d779..a0370bdb6 100644 --- a/src/allmydata/test/test_istorageserver.py +++ b/src/allmydata/test/test_istorageserver.py @@ -440,6 +440,17 @@ class IStorageServerImmutableAPIsTestsMixin(object): b"immutable", storage_index, 0, b"ono" ) + @inlineCallbacks + def test_advise_corrupt_share_unknown_share_number(self): + """ + Calling ``advise_corrupt_share()`` on an immutable share, with an + unknown share number, does not result in error. + """ + storage_index, _, _ = yield self.create_share() + yield self.storage_client.advise_corrupt_share( + b"immutable", storage_index, 999, b"ono" + ) + @inlineCallbacks def test_allocate_buckets_creates_lease(self): """ @@ -909,6 +920,19 @@ class IStorageServerMutableAPIsTestsMixin(object): b"mutable", storage_index, 0, b"ono" ) + @inlineCallbacks + def test_advise_corrupt_share_unknown_share_number(self): + """ + Calling ``advise_corrupt_share()`` on a mutable share with an unknown + share number does not result in error (other behavior is opaque at this + level of abstraction). + """ + secrets, storage_index = yield self.create_slot() + + yield self.storage_client.advise_corrupt_share( + b"mutable", storage_index, 999, b"ono" + ) + @inlineCallbacks def test_STARAW_create_lease(self): """ From 0d23237b11aea61241a75e4d19c6df394b9de0b2 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 13 Oct 2022 13:44:49 -0400 Subject: [PATCH 18/47] 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): From b80a215ae1dc80a3760049bec864fe227eee1654 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 13 Oct 2022 13:56:28 -0400 Subject: [PATCH 19/47] test_rref passes now. --- src/allmydata/storage_client.py | 8 ++++---- src/allmydata/test/common_system.py | 2 ++ src/allmydata/test/test_system.py | 3 ++- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index d492ee4cf..6f2106f87 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -1052,10 +1052,9 @@ class HTTPNativeStorageServer(service.MultiService): """ See ``IServer.get_storage_server``. """ - if self.is_connected(): - return self._istorage_server - else: + if self._connection_status.summary == "unstarted": return None + return self._istorage_server def stop_connecting(self): self._lc.stop() @@ -1070,10 +1069,11 @@ class HTTPNativeStorageServer(service.MultiService): ) def stopService(self): - service.MultiService.stopService(self) + result = service.MultiService.stopService(self) if self._lc.running: self._lc.stop() self._failed_to_connect("shut down") + return result class UnknownServerTypeError(Exception): diff --git a/src/allmydata/test/common_system.py b/src/allmydata/test/common_system.py index ef4b65529..ee345a0c0 100644 --- a/src/allmydata/test/common_system.py +++ b/src/allmydata/test/common_system.py @@ -21,6 +21,7 @@ from functools import partial from twisted.internet import reactor from twisted.internet import defer from twisted.internet.defer import inlineCallbacks +from twisted.internet.task import deferLater from twisted.application import service from foolscap.api import flushEventualQueue @@ -658,6 +659,7 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): log.msg("shutting down SystemTest services") d = self.sparent.stopService() d.addBoth(flush_but_dont_ignore) + d.addBoth(lambda x: deferLater(reactor, 0.01, lambda: x)) return d def getdir(self, subdir): diff --git a/src/allmydata/test/test_system.py b/src/allmydata/test/test_system.py index d94b4d163..c6d2c6bb7 100644 --- a/src/allmydata/test/test_system.py +++ b/src/allmydata/test/test_system.py @@ -1821,9 +1821,10 @@ class Connections(SystemTestMixin, unittest.TestCase): # now shut down the server d.addCallback(lambda ign: self.clients[1].disownServiceParent()) + # and wait for the client to notice def _poll(): - return len(self.c0.storage_broker.get_connected_servers()) < 2 + return len(self.c0.storage_broker.get_connected_servers()) == 1 d.addCallback(lambda ign: self.poll(_poll)) def _down(ign): From 0f31e3cd4b054b17076ffeaa73cc412bc63191b3 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 13 Oct 2022 14:41:59 -0400 Subject: [PATCH 20/47] Leave HTTP off by default for now. --- src/allmydata/node.py | 8 ++++++-- src/allmydata/test/common_system.py | 5 ++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/allmydata/node.py b/src/allmydata/node.py index 7d33d220a..f572cf7d9 100644 --- a/src/allmydata/node.py +++ b/src/allmydata/node.py @@ -908,12 +908,16 @@ def create_main_tub(config, tub_options, # FIXME? "node.pem" was the CERTFILE option/thing certfile = config.get_private_path("node.pem") - tub = create_tub( tub_options, default_connection_handlers, foolscap_connection_handlers, - force_foolscap=config.get_config("node", "force_foolscap", False), + # TODO eventually we will want the default to be False, but for now we + # don't want to enable HTTP by default. + # https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3934 + force_foolscap=config.get_config( + "node", "force_foolscap", default=True, boolean=True + ), handler_overrides=handler_overrides, certFile=certfile, ) diff --git a/src/allmydata/test/common_system.py b/src/allmydata/test/common_system.py index ee345a0c0..edeea5689 100644 --- a/src/allmydata/test/common_system.py +++ b/src/allmydata/test/common_system.py @@ -794,13 +794,13 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): if which in feature_matrix.get((section, feature), {which}): config.setdefault(section, {})[feature] = value - if force_foolscap: - config.setdefault("node", {})["force_foolscap"] = force_foolscap + #config.setdefault("node", {})["force_foolscap"] = force_foolscap setnode = partial(setconf, config, which, "node") sethelper = partial(setconf, config, which, "helper") setnode("nickname", u"client %d \N{BLACK SMILING FACE}" % (which,)) + setnode("force_foolscap", str(force_foolscap)) tub_location_hint, tub_port_endpoint = self.port_assigner.assign(reactor) setnode("tub.port", tub_port_endpoint) @@ -818,7 +818,6 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): " furl: %s\n") % self.introducer_furl iyaml_fn = os.path.join(basedir, "private", "introducers.yaml") fileutil.write(iyaml_fn, iyaml) - return _render_config(config) def _set_up_client_node(self, which, force_foolscap): From 42d38433436a0f7650704fd45383688f4eeb9ac1 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 14 Oct 2022 09:16:59 -0400 Subject: [PATCH 21/47] Run test_system with both Foolscap and HTTP storage protocols, plus some resulting cleanups. --- src/allmydata/test/common_system.py | 37 +++++++------ src/allmydata/test/test_istorageserver.py | 65 +++++++++-------------- src/allmydata/test/test_system.py | 17 +++++- 3 files changed, 63 insertions(+), 56 deletions(-) diff --git a/src/allmydata/test/common_system.py b/src/allmydata/test/common_system.py index edeea5689..96ab4e093 100644 --- a/src/allmydata/test/common_system.py +++ b/src/allmydata/test/common_system.py @@ -5,16 +5,7 @@ in ``allmydata.test.test_system``. Ported to Python 3. """ -from __future__ import absolute_import -from __future__ import division -from __future__ import print_function -from __future__ import unicode_literals - -from future.utils import PY2 -if PY2: - # Don't import bytes since it causes issues on (so far unported) modules on Python 2. - from future.builtins import filter, map, zip, ascii, chr, hex, input, next, oct, open, pow, round, super, dict, list, object, range, max, min, str # noqa: F401 - +from typing import Optional import os from functools import partial @@ -30,6 +21,10 @@ 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 allmydata.storage_client import ( + NativeStorageServer, + HTTPNativeStorageServer, +) from twisted.python.filepath import ( FilePath, @@ -646,6 +641,11 @@ def _render_section_values(values): class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): + # If set to True, use Foolscap for storage protocol. If set to False, HTTP + # will be used when possible. If set to None, this suggests a bug in the + # test code. + FORCE_FOOLSCAP_FOR_STORAGE : Optional[bool] = None + def setUp(self): http_client.StorageClient.start_test_mode() self.port_assigner = SameProcessStreamEndpointAssigner() @@ -702,7 +702,7 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): return f.read().strip() @inlineCallbacks - def set_up_nodes(self, NUMCLIENTS=5, force_foolscap=False): + def set_up_nodes(self, NUMCLIENTS=5): """ Create an introducer and ``NUMCLIENTS`` client nodes pointed at it. All of the nodes are running in this process. @@ -715,18 +715,25 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): :param int NUMCLIENTS: The number of client nodes to create. - :param bool force_foolscap: Force clients to use Foolscap instead of e.g. - HTTPS when available. - :return: A ``Deferred`` that fires when the nodes have connected to each other. """ + self.assertIn( + self.FORCE_FOOLSCAP_FOR_STORAGE, (True, False), + "You forgot to set FORCE_FOOLSCAP_FOR_STORAGE on {}".format(self.__class__) + ) self.numclients = NUMCLIENTS self.introducer = yield self._create_introducer() self.add_service(self.introducer) self.introweb_url = self._get_introducer_web() - yield self._set_up_client_nodes(force_foolscap) + yield self._set_up_client_nodes(self.FORCE_FOOLSCAP_FOR_STORAGE) + native_server = next(iter(self.clients[0].storage_broker.get_known_servers())) + if self.FORCE_FOOLSCAP_FOR_STORAGE: + expected_storage_server_class = NativeStorageServer + else: + expected_storage_server_class = HTTPNativeStorageServer + self.assertIsInstance(native_server, expected_storage_server_class) @inlineCallbacks def _set_up_client_nodes(self, force_foolscap): diff --git a/src/allmydata/test/test_istorageserver.py b/src/allmydata/test/test_istorageserver.py index a0370bdb6..a488622c7 100644 --- a/src/allmydata/test/test_istorageserver.py +++ b/src/allmydata/test/test_istorageserver.py @@ -1046,13 +1046,12 @@ class _SharedMixin(SystemTestMixin): """Base class for Foolscap and HTTP mixins.""" SKIP_TESTS = set() # type: Set[str] - FORCE_FOOLSCAP = False - - def _get_native_server(self): - return next(iter(self.clients[0].storage_broker.get_known_servers())) def _get_istorage_server(self): - raise NotImplementedError("implement in subclass") + native_server = next(iter(self.clients[0].storage_broker.get_known_servers())) + client = native_server.get_storage_server() + self.assertTrue(IStorageServer.providedBy(client)) + return client @inlineCallbacks def setUp(self): @@ -1065,7 +1064,7 @@ class _SharedMixin(SystemTestMixin): self.basedir = "test_istorageserver/" + self.id() yield SystemTestMixin.setUp(self) - yield self.set_up_nodes(1, self.FORCE_FOOLSCAP) + yield self.set_up_nodes(1) self.server = None for s in self.clients[0].services: if isinstance(s, StorageServer): @@ -1075,7 +1074,7 @@ class _SharedMixin(SystemTestMixin): self._clock = Clock() self._clock.advance(123456) self.server._clock = self._clock - self.storage_client = yield self._get_istorage_server() + self.storage_client = self._get_istorage_server() def fake_time(self): """Return the current fake, test-controlled, time.""" @@ -1091,49 +1090,29 @@ class _SharedMixin(SystemTestMixin): yield SystemTestMixin.tearDown(self) -class _FoolscapMixin(_SharedMixin): - """Run tests on Foolscap version of ``IStorageServer``.""" - - FORCE_FOOLSCAP = True - - def _get_istorage_server(self): - native_server = self._get_native_server() - assert isinstance(native_server, NativeStorageServer) - client = native_server.get_storage_server() - self.assertTrue(IStorageServer.providedBy(client)) - return succeed(client) - - -class _HTTPMixin(_SharedMixin): - """Run tests on the HTTP version of ``IStorageServer``.""" - - FORCE_FOOLSCAP = False - - def _get_istorage_server(self): - native_server = self._get_native_server() - assert isinstance(native_server, HTTPNativeStorageServer) - client = native_server.get_storage_server() - self.assertTrue(IStorageServer.providedBy(client)) - return succeed(client) - - class FoolscapSharedAPIsTests( - _FoolscapMixin, IStorageServerSharedAPIsTestsMixin, AsyncTestCase + _SharedMixin, IStorageServerSharedAPIsTestsMixin, AsyncTestCase ): """Foolscap-specific tests for shared ``IStorageServer`` APIs.""" + FORCE_FOOLSCAP_FOR_STORAGE = True + class HTTPSharedAPIsTests( - _HTTPMixin, IStorageServerSharedAPIsTestsMixin, AsyncTestCase + _SharedMixin, IStorageServerSharedAPIsTestsMixin, AsyncTestCase ): """HTTP-specific tests for shared ``IStorageServer`` APIs.""" + FORCE_FOOLSCAP_FOR_STORAGE = False + class FoolscapImmutableAPIsTests( - _FoolscapMixin, IStorageServerImmutableAPIsTestsMixin, AsyncTestCase + _SharedMixin, IStorageServerImmutableAPIsTestsMixin, AsyncTestCase ): """Foolscap-specific tests for immutable ``IStorageServer`` APIs.""" + FORCE_FOOLSCAP_FOR_STORAGE = True + def test_disconnection(self): """ If we disconnect in the middle of writing to a bucket, all data is @@ -1156,23 +1135,29 @@ class FoolscapImmutableAPIsTests( """ current = self.storage_client yield self.bounce_client(0) - self.storage_client = self._get_native_server().get_storage_server() + self.storage_client = self._get_istorage_server() assert self.storage_client is not current class HTTPImmutableAPIsTests( - _HTTPMixin, IStorageServerImmutableAPIsTestsMixin, AsyncTestCase + _SharedMixin, IStorageServerImmutableAPIsTestsMixin, AsyncTestCase ): """HTTP-specific tests for immutable ``IStorageServer`` APIs.""" + FORCE_FOOLSCAP_FOR_STORAGE = False + class FoolscapMutableAPIsTests( - _FoolscapMixin, IStorageServerMutableAPIsTestsMixin, AsyncTestCase + _SharedMixin, IStorageServerMutableAPIsTestsMixin, AsyncTestCase ): """Foolscap-specific tests for mutable ``IStorageServer`` APIs.""" + FORCE_FOOLSCAP_FOR_STORAGE = True + class HTTPMutableAPIsTests( - _HTTPMixin, IStorageServerMutableAPIsTestsMixin, AsyncTestCase + _SharedMixin, IStorageServerMutableAPIsTestsMixin, AsyncTestCase ): """HTTP-specific tests for mutable ``IStorageServer`` APIs.""" + + FORCE_FOOLSCAP_FOR_STORAGE = False diff --git a/src/allmydata/test/test_system.py b/src/allmydata/test/test_system.py index c6d2c6bb7..a83ff9488 100644 --- a/src/allmydata/test/test_system.py +++ b/src/allmydata/test/test_system.py @@ -117,7 +117,8 @@ class CountingDataUploadable(upload.Data): class SystemTest(SystemTestMixin, RunBinTahoeMixin, unittest.TestCase): - + """Foolscap integration-y tests.""" + FORCE_FOOLSCAP_FOR_STORAGE = True timeout = 180 def test_connections(self): @@ -1794,6 +1795,7 @@ class SystemTest(SystemTestMixin, RunBinTahoeMixin, unittest.TestCase): class Connections(SystemTestMixin, unittest.TestCase): + FORCE_FOOLSCAP_FOR_STORAGE = True def test_rref(self): # The way the listening port is created is via @@ -1834,3 +1836,16 @@ class Connections(SystemTestMixin, unittest.TestCase): self.assertEqual(storage_server, self.s1_storage_server) d.addCallback(_down) return d + + +class HTTPSystemTest(SystemTest): + """HTTP storage protocol variant of the system tests.""" + + FORCE_FOOLSCAP_FOR_STORAGE = False + + + +class HTTPConnections(Connections): + """HTTP storage protocol variant of the connections tests.""" + FORCE_FOOLSCAP_FOR_STORAGE = False + From e409262e86ff3639187bfa89f438b6e9db071228 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 14 Oct 2022 09:50:07 -0400 Subject: [PATCH 22/47] Fix some flakes. --- src/allmydata/test/test_istorageserver.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/allmydata/test/test_istorageserver.py b/src/allmydata/test/test_istorageserver.py index a488622c7..9e7e7b6e1 100644 --- a/src/allmydata/test/test_istorageserver.py +++ b/src/allmydata/test/test_istorageserver.py @@ -15,7 +15,7 @@ from typing import Set from random import Random from unittest import SkipTest -from twisted.internet.defer import inlineCallbacks, returnValue, succeed +from twisted.internet.defer import inlineCallbacks, returnValue from twisted.internet.task import Clock from foolscap.api import Referenceable, RemoteException @@ -25,10 +25,6 @@ from allmydata.interfaces import IStorageServer from .common_system import SystemTestMixin from .common import AsyncTestCase from allmydata.storage.server import StorageServer # not a IStorageServer!! -from allmydata.storage_client import ( - NativeStorageServer, - HTTPNativeStorageServer, -) # Use random generator with known seed, so results are reproducible if tests From 0febc8745653992cbb53d98702c92edc24b7a516 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 14 Oct 2022 10:03:06 -0400 Subject: [PATCH 23/47] Don't include reactor in comparison. --- src/allmydata/storage/http_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index 2589d4e41..40979d3cb 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -301,7 +301,7 @@ class StorageClient(object): _base_url: DecodedURL _swissnum: bytes _treq: Union[treq, StubTreq, HTTPClient] = field(eq=False) - _clock: IReactorTime + _clock: IReactorTime = field(eq=False) @classmethod def from_nurl( From f68c3978f616c5efecc15094aa83c363bd6db58d Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 14 Oct 2022 10:18:38 -0400 Subject: [PATCH 24/47] News fragment. --- newsfragments/3783.minor | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 newsfragments/3783.minor diff --git a/newsfragments/3783.minor b/newsfragments/3783.minor new file mode 100644 index 000000000..e69de29bb From 48ae729c0de57818d132763aa62e99faffd46556 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 2 Nov 2022 10:18:23 -0400 Subject: [PATCH 25/47] Don't reuse basedir across tests. --- src/allmydata/test/test_system.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/allmydata/test/test_system.py b/src/allmydata/test/test_system.py index a83ff9488..f03d795ba 100644 --- a/src/allmydata/test/test_system.py +++ b/src/allmydata/test/test_system.py @@ -121,8 +121,13 @@ class SystemTest(SystemTestMixin, RunBinTahoeMixin, unittest.TestCase): FORCE_FOOLSCAP_FOR_STORAGE = True timeout = 180 + @property + def basedir(self): + return "system/SystemTest/{}-foolscap-{}".format( + self.id().split(".")[-1], self.FORCE_FOOLSCAP_FOR_STORAGE + ) + def test_connections(self): - self.basedir = "system/SystemTest/test_connections" d = self.set_up_nodes() self.extra_node = None d.addCallback(lambda res: self.add_extra_node(self.numclients)) @@ -150,11 +155,9 @@ class SystemTest(SystemTestMixin, RunBinTahoeMixin, unittest.TestCase): del test_connections def test_upload_and_download_random_key(self): - self.basedir = "system/SystemTest/test_upload_and_download_random_key" return self._test_upload_and_download(convergence=None) def test_upload_and_download_convergent(self): - self.basedir = "system/SystemTest/test_upload_and_download_convergent" return self._test_upload_and_download(convergence=b"some convergence string") def _test_upload_and_download(self, convergence): @@ -517,7 +520,6 @@ class SystemTest(SystemTestMixin, RunBinTahoeMixin, unittest.TestCase): def test_mutable(self): - self.basedir = "system/SystemTest/test_mutable" DATA = b"initial contents go here." # 25 bytes % 3 != 0 DATA_uploadable = MutableData(DATA) NEWDATA = b"new contents yay" @@ -747,7 +749,6 @@ class SystemTest(SystemTestMixin, RunBinTahoeMixin, unittest.TestCase): # plaintext_hash check. def test_filesystem(self): - self.basedir = "system/SystemTest/test_filesystem" self.data = LARGE_DATA d = self.set_up_nodes() def _new_happy_semantics(ign): @@ -1714,7 +1715,6 @@ class SystemTest(SystemTestMixin, RunBinTahoeMixin, unittest.TestCase): def test_filesystem_with_cli_in_subprocess(self): # We do this in a separate test so that test_filesystem doesn't skip if we can't run bin/tahoe. - self.basedir = "system/SystemTest/test_filesystem_with_cli_in_subprocess" d = self.set_up_nodes() def _new_happy_semantics(ign): for c in self.clients: @@ -1807,7 +1807,9 @@ class Connections(SystemTestMixin, unittest.TestCase): # 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" + self.basedir = "system/Connections/rref-foolscap-{}".format( + self.FORCE_FOOLSCAP_FOR_STORAGE + ) d = self.set_up_nodes(2) def _start(ign): self.c0 = self.clients[0] From e05136c2385d222bd50413054dc8ac2a9d60d243 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 2 Nov 2022 13:13:21 -0400 Subject: [PATCH 26/47] Less aggressive timeout, to try to make tests pass on CI. --- src/allmydata/storage/http_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index adc3e1525..e520088c3 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -405,7 +405,7 @@ class StorageClient(object): # 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) + result.addTimeout(5, self._clock) return result From db59eb12c092264f357c59afc3586dcb8259d0f8 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 2 Nov 2022 15:22:36 -0400 Subject: [PATCH 27/47] Increase timeout. --- .circleci/run-tests.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/run-tests.sh b/.circleci/run-tests.sh index 764651c40..854013c32 100755 --- a/.circleci/run-tests.sh +++ b/.circleci/run-tests.sh @@ -52,7 +52,7 @@ fi # This is primarily aimed at catching hangs on the PyPy job which runs for # about 21 minutes and then gets killed by CircleCI in a way that fails the # job and bypasses our "allowed failure" logic. -TIMEOUT="timeout --kill-after 1m 15m" +TIMEOUT="timeout --kill-after 1m 25m" # Run the test suite as a non-root user. This is the expected usage some # small areas of the test suite assume non-root privileges (such as unreadable From 262d9d85b97cb064da47c82bab22e62b48db6cd4 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 3 Nov 2022 14:14:21 -0400 Subject: [PATCH 28/47] Switch to using persistent connections in tests too. --- src/allmydata/storage/http_client.py | 34 +++++++++++++++------------- src/allmydata/test/common_system.py | 10 +++++++- src/allmydata/test/test_system.py | 3 +++ 3 files changed, 30 insertions(+), 17 deletions(-) diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index e520088c3..96820d4a5 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -282,19 +282,20 @@ class StorageClient(object): Low-level HTTP client that talks to the HTTP storage server. """ - # If True, we're doing unit testing. - TEST_MODE = False + # If set, we're doing unit testing and we should call this with + # HTTPConnectionPool we create. + TEST_MODE_REGISTER_HTTP_POOL = None @classmethod - def start_test_mode(cls): + def start_test_mode(cls, callback): """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. + In testing mode we register the pool with test system using the given + callback so it can Do Things, most notably killing off idle HTTP + connections at test shutdown and, in some tests, in the midddle of the + test. """ - cls.TEST_MODE = True + cls.TEST_MODE_REGISTER_HTTP_POOL = callback # The URL is a HTTPS URL ("https://..."). To construct from a NURL, use # ``StorageClient.from_nurl()``. @@ -318,13 +319,10 @@ class StorageClient(object): assert nurl.scheme == "pb" swissnum = nurl.path[0].encode("ascii") certificate_hash = nurl.user.encode("ascii") + pool = HTTPConnectionPool(reactor) - if cls.TEST_MODE: - pool = HTTPConnectionPool(reactor, persistent=False) - pool.retryAutomatically = False - pool.maxPersistentPerHost = 0 - else: - pool = HTTPConnectionPool(reactor) + if cls.TEST_MODE_REGISTER_HTTP_POOL is not None: + cls.TEST_MODE_REGISTER_HTTP_POOL(pool) treq_client = HTTPClient( Agent( @@ -403,8 +401,12 @@ class StorageClient(object): 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: + # test_rref in test_system.py. Unfortunately, test_rref results in the + # socket still listening(!), only without an HTTP server, due to limits + # in the relevant socket-binding test setup code. As a result, we don't + # get connection refused, the client will successfully connect. So we + # want a timeout so we notice that. + if self.TEST_MODE_REGISTER_HTTP_POOL is not None: result.addTimeout(5, self._clock) return result diff --git a/src/allmydata/test/common_system.py b/src/allmydata/test/common_system.py index 96ab4e093..f47aad3b6 100644 --- a/src/allmydata/test/common_system.py +++ b/src/allmydata/test/common_system.py @@ -647,7 +647,8 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): FORCE_FOOLSCAP_FOR_STORAGE : Optional[bool] = None def setUp(self): - http_client.StorageClient.start_test_mode() + self._http_client_pools = [] + http_client.StorageClient.start_test_mode(self._http_client_pools.append) self.port_assigner = SameProcessStreamEndpointAssigner() self.port_assigner.setUp() self.addCleanup(self.port_assigner.tearDown) @@ -655,10 +656,17 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): self.sparent = service.MultiService() self.sparent.startService() + def close_idle_http_connections(self): + """Close all HTTP client connections that are just hanging around.""" + return defer.gatherResults( + [pool.closeCachedConnections() for pool in self._http_client_pools] + ) + def tearDown(self): log.msg("shutting down SystemTest services") d = self.sparent.stopService() d.addBoth(flush_but_dont_ignore) + d.addBoth(lambda x: self.close_idle_http_connections().addCallback(lambda _: x)) d.addBoth(lambda x: deferLater(reactor, 0.01, lambda: x)) return d diff --git a/src/allmydata/test/test_system.py b/src/allmydata/test/test_system.py index f03d795ba..670ac5868 100644 --- a/src/allmydata/test/test_system.py +++ b/src/allmydata/test/test_system.py @@ -1826,6 +1826,9 @@ class Connections(SystemTestMixin, unittest.TestCase): # now shut down the server d.addCallback(lambda ign: self.clients[1].disownServiceParent()) + # kill any persistent http connections that might continue to work + d.addCallback(lambda ign: self.close_idle_http_connections()) + # and wait for the client to notice def _poll(): return len(self.c0.storage_broker.get_connected_servers()) == 1 From 8bebb09edd2026a77dd6f8081a1fe7c0069071b3 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 3 Nov 2022 14:38:59 -0400 Subject: [PATCH 29/47] Less test-specific way to make test_rref pass. --- src/allmydata/storage/http_client.py | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index 96820d4a5..7fcf8114c 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -398,18 +398,7 @@ class StorageClient(object): kwargs["data"] = dumps(message_to_serialize) headers.addRawHeader("Content-Type", CBOR_MIME_TYPE) - 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. Unfortunately, test_rref results in the - # socket still listening(!), only without an HTTP server, due to limits - # in the relevant socket-binding test setup code. As a result, we don't - # get connection refused, the client will successfully connect. So we - # want a timeout so we notice that. - if self.TEST_MODE_REGISTER_HTTP_POOL is not None: - result.addTimeout(5, self._clock) - - return result + return self._treq.request(method, url, headers=headers, **kwargs) @define(hash=True) @@ -426,7 +415,12 @@ class StorageClientGeneral(object): Return the version metadata for the server. """ url = self._client.relative_url("/storage/v1/version") - response = yield self._client.request("GET", url) + result = self._client.request("GET", url) + # 1. Getting the version should never take particularly long. + # 2. Clients rely on the version command for liveness checks of servers. + result.addTimeout(5, self._client._clock) + + response = yield result decoded_response = yield _decode_cbor(response, _SCHEMAS["get_version"]) returnValue(decoded_response) From 1e50e96e2456910598862e64f7585a6dd47d59f2 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 3 Nov 2022 15:04:41 -0400 Subject: [PATCH 30/47] Update to new test API. --- src/allmydata/test/test_storage_http.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py index 819c94f83..25c21e03f 100644 --- a/src/allmydata/test/test_storage_http.py +++ b/src/allmydata/test/test_storage_http.py @@ -291,7 +291,9 @@ class CustomHTTPServerTests(SyncTestCase): def setUp(self): super(CustomHTTPServerTests, self).setUp() - StorageClient.start_test_mode() + StorageClient.start_test_mode( + lambda pool: self.addCleanup(pool.closeCachedConnections) + ) # Could be a fixture, but will only be used in this test class so not # going to bother: self._http_server = TestApp() @@ -299,7 +301,7 @@ class CustomHTTPServerTests(SyncTestCase): DecodedURL.from_text("http://127.0.0.1"), SWISSNUM_FOR_TEST, treq=StubTreq(self._http_server._app.resource()), - clock=Clock() + clock=Clock(), ) def test_authorization_enforcement(self): @@ -377,7 +379,9 @@ class HttpTestFixture(Fixture): """ def _setUp(self): - StorageClient.start_test_mode() + StorageClient.start_test_mode( + lambda pool: self.addCleanup(pool.closeCachedConnections) + ) self.clock = Clock() self.tempdir = self.useFixture(TempDir()) # The global Cooperator used by Twisted (a) used by pull producers in @@ -1446,7 +1450,9 @@ class SharedImmutableMutableTestsMixin: self.http.client.request( "GET", self.http.client.relative_url( - "/storage/v1/{}/{}/1".format(self.KIND, _encode_si(storage_index)) + "/storage/v1/{}/{}/1".format( + self.KIND, _encode_si(storage_index) + ) ), headers=headers, ) From 414b4635569145ed277bfe0e0e540d62430e0bb8 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Mon, 7 Nov 2022 09:23:04 -0500 Subject: [PATCH 31/47] Use built-in treq timeout feature. --- src/allmydata/storage/http_client.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index 7fcf8114c..d6121aba2 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -415,12 +415,10 @@ class StorageClientGeneral(object): Return the version metadata for the server. """ url = self._client.relative_url("/storage/v1/version") - result = self._client.request("GET", url) # 1. Getting the version should never take particularly long. # 2. Clients rely on the version command for liveness checks of servers. - result.addTimeout(5, self._client._clock) - - response = yield result + # Thus, a short timeout. + response = yield self._client.request("GET", url, timeout=5) decoded_response = yield _decode_cbor(response, _SCHEMAS["get_version"]) returnValue(decoded_response) From 8d678fe3de4dacdf206e737ef130a91b92004656 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Mon, 7 Nov 2022 11:41:50 -0500 Subject: [PATCH 32/47] Increase timeout. --- src/allmydata/test/common_system.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/test/common_system.py b/src/allmydata/test/common_system.py index f47aad3b6..90990a8ca 100644 --- a/src/allmydata/test/common_system.py +++ b/src/allmydata/test/common_system.py @@ -667,7 +667,7 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): d = self.sparent.stopService() d.addBoth(flush_but_dont_ignore) d.addBoth(lambda x: self.close_idle_http_connections().addCallback(lambda _: x)) - d.addBoth(lambda x: deferLater(reactor, 0.01, lambda: x)) + d.addBoth(lambda x: deferLater(reactor, 0.02, lambda: x)) return d def getdir(self, subdir): From d1287df62990d7c096e1935718c2f048d1a2039d Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 15 Nov 2022 14:02:19 -0500 Subject: [PATCH 33/47] The short timeout should be specific to the storage client's needs. --- src/allmydata/storage/http_client.py | 5 +---- src/allmydata/storage_client.py | 8 ++++++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index d6121aba2..d468d2436 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -415,10 +415,7 @@ class StorageClientGeneral(object): Return the version metadata for the server. """ url = self._client.relative_url("/storage/v1/version") - # 1. Getting the version should never take particularly long. - # 2. Clients rely on the version command for liveness checks of servers. - # Thus, a short timeout. - response = yield self._client.request("GET", url, timeout=5) + response = yield self._client.request("GET", url) decoded_response = yield _decode_cbor(response, _SCHEMAS["get_version"]) returnValue(decoded_response) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 6f2106f87..140e29607 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -944,12 +944,13 @@ class HTTPNativeStorageServer(service.MultiService): "connected". """ - def __init__(self, server_id: bytes, announcement): + def __init__(self, server_id: bytes, announcement, reactor=reactor): service.MultiService.__init__(self) assert isinstance(server_id, bytes) self._server_id = server_id self.announcement = announcement self._on_status_changed = ObserverList() + self._reactor = reactor furl = announcement["anonymous-storage-FURL"].encode("utf-8") ( self._nickname, @@ -1063,7 +1064,10 @@ class HTTPNativeStorageServer(service.MultiService): self._connect() def _connect(self): - return self._istorage_server.get_version().addCallbacks( + result = self._istorage_server.get_version() + # Set a short timeout since we're relying on this for server liveness. + result.addTimeout(5, self._reactor) + result.addCallbacks( self._got_version, self._failed_to_connect ) From 6c80ad5290c634a3395a3c5a222a15f6ed9f0abe Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 15 Nov 2022 14:13:50 -0500 Subject: [PATCH 34/47] Not necessary. --- src/allmydata/storage/http_client.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index d468d2436..f0b45742c 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -301,8 +301,8 @@ class StorageClient(object): # ``StorageClient.from_nurl()``. _base_url: DecodedURL _swissnum: bytes - _treq: Union[treq, StubTreq, HTTPClient] = field(eq=False) - _clock: IReactorTime = field(eq=False) + _treq: Union[treq, StubTreq, HTTPClient] + _clock: IReactorTime @classmethod def from_nurl( From d700163aecda5ff23b772c561b5f9a1992b45f82 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 15 Nov 2022 14:14:29 -0500 Subject: [PATCH 35/47] Remove no-longer-relevant comment. --- src/allmydata/storage/http_client.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index f0b45742c..cc26d4b37 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -312,8 +312,6 @@ class StorageClient(object): ) -> StorageClient: """ Create a ``StorageClient`` for the given NURL. - - ``persistent`` indicates whether to use persistent HTTP connections. """ assert nurl.fragment == "v=1" assert nurl.scheme == "pb" From 4aeb62b66c12e5d337d6ebeeb26cea8f3f3ff13d Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 15 Nov 2022 14:16:41 -0500 Subject: [PATCH 36/47] Use a constant. --- src/allmydata/client.py | 2 +- src/allmydata/storage_client.py | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/allmydata/client.py b/src/allmydata/client.py index aa03015fc..1e28bb98b 100644 --- a/src/allmydata/client.py +++ b/src/allmydata/client.py @@ -826,7 +826,7 @@ class _Client(node.Node, pollmixin.PollMixin): if hasattr(self.tub.negotiationClass, "add_storage_server"): nurls = self.tub.negotiationClass.add_storage_server(ss, swissnum.encode("ascii")) self.storage_nurls = nurls - announcement["anonymous-storage-NURLs"] = [n.to_text() for n in nurls] + announcement[storage_client.ANONYMOUS_STORAGE_NURLS] = [n.to_text() for n in nurls] announcement["anonymous-storage-FURL"] = furl enabled_storage_servers = self._enable_storage_servers( diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 140e29607..59d3406f1 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -80,6 +80,8 @@ from allmydata.storage.http_client import ( ReadVector, TestWriteVectors, WriteVector, TestVector, ClientException ) +ANONYMOUS_STORAGE_NURLS = "anonymous-storage-NURLs" + # who is responsible for de-duplication? # both? @@ -267,8 +269,7 @@ class StorageFarmBroker(service.MultiService): by the given announcement. """ assert isinstance(server_id, bytes) - # TODO use constant for anonymous-storage-NURLs - if len(server["ann"].get("anonymous-storage-NURLs", [])) > 0: + if len(server["ann"].get(ANONYMOUS_STORAGE_NURLS, [])) > 0: s = HTTPNativeStorageServer(server_id, server["ann"]) s.on_status_changed(lambda _: self._got_connection()) return s @@ -961,7 +962,7 @@ class HTTPNativeStorageServer(service.MultiService): ) = _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]) + nurl = DecodedURL.from_text(announcement[ANONYMOUS_STORAGE_NURLS][0]) self._istorage_server = _HTTPStorageServer.from_http_client( StorageClient.from_nurl(nurl, reactor) ) From 8e4ac6903298e8081daf4d1947c569d02111d160 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 15 Nov 2022 14:21:31 -0500 Subject: [PATCH 37/47] Stop test mode when done. --- src/allmydata/storage/http_client.py | 5 +++++ src/allmydata/test/common_system.py | 1 + src/allmydata/test/test_storage_http.py | 2 ++ 3 files changed, 8 insertions(+) diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index cc26d4b37..fed66bb75 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -297,6 +297,11 @@ class StorageClient(object): """ cls.TEST_MODE_REGISTER_HTTP_POOL = callback + @classmethod + def stop_test_mode(cls): + """Stop testing mode.""" + cls.TEST_MODE_REGISTER_HTTP_POOL = None + # The URL is a HTTPS URL ("https://..."). To construct from a NURL, use # ``StorageClient.from_nurl()``. _base_url: DecodedURL diff --git a/src/allmydata/test/common_system.py b/src/allmydata/test/common_system.py index 90990a8ca..af86440cc 100644 --- a/src/allmydata/test/common_system.py +++ b/src/allmydata/test/common_system.py @@ -649,6 +649,7 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): def setUp(self): self._http_client_pools = [] http_client.StorageClient.start_test_mode(self._http_client_pools.append) + self.addCleanup(http_client.StorageClient.stop_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 25c21e03f..87a6a2306 100644 --- a/src/allmydata/test/test_storage_http.py +++ b/src/allmydata/test/test_storage_http.py @@ -294,6 +294,7 @@ class CustomHTTPServerTests(SyncTestCase): StorageClient.start_test_mode( lambda pool: self.addCleanup(pool.closeCachedConnections) ) + self.addCleanup(StorageClient.stop_test_mode) # Could be a fixture, but will only be used in this test class so not # going to bother: self._http_server = TestApp() @@ -382,6 +383,7 @@ class HttpTestFixture(Fixture): StorageClient.start_test_mode( lambda pool: self.addCleanup(pool.closeCachedConnections) ) + self.addCleanup(StorageClient.stop_test_mode) self.clock = Clock() self.tempdir = self.useFixture(TempDir()) # The global Cooperator used by Twisted (a) used by pull producers in From fb52b4d302d6f717a4393a518ddbe8fb773e406c Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 15 Nov 2022 14:22:30 -0500 Subject: [PATCH 38/47] Delete some garbage. --- src/allmydata/test/common_system.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/allmydata/test/common_system.py b/src/allmydata/test/common_system.py index af86440cc..0c7d7f747 100644 --- a/src/allmydata/test/common_system.py +++ b/src/allmydata/test/common_system.py @@ -810,8 +810,6 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): if which in feature_matrix.get((section, feature), {which}): config.setdefault(section, {})[feature] = value - #config.setdefault("node", {})["force_foolscap"] = force_foolscap - setnode = partial(setconf, config, which, "node") sethelper = partial(setconf, config, which, "helper") From f3fc4268309316e9200f251df64b27a7bca5f33e Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 15 Nov 2022 14:36:14 -0500 Subject: [PATCH 39/47] Switch to [storage] force_foolscap. --- src/allmydata/client.py | 1 + src/allmydata/node.py | 3 +-- src/allmydata/test/common_system.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/allmydata/client.py b/src/allmydata/client.py index 1e28bb98b..1a158a1aa 100644 --- a/src/allmydata/client.py +++ b/src/allmydata/client.py @@ -104,6 +104,7 @@ _client_config = configutil.ValidConfiguration( "reserved_space", "storage_dir", "plugins", + "force_foolscap", ), "sftpd": ( "accounts.file", diff --git a/src/allmydata/node.py b/src/allmydata/node.py index f572cf7d9..8266fe3fb 100644 --- a/src/allmydata/node.py +++ b/src/allmydata/node.py @@ -64,7 +64,6 @@ def _common_valid_config(): "tcp", ), "node": ( - "force_foolscap", "log_gatherer.furl", "nickname", "reveal-ip-address", @@ -916,7 +915,7 @@ def create_main_tub(config, tub_options, # don't want to enable HTTP by default. # https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3934 force_foolscap=config.get_config( - "node", "force_foolscap", default=True, boolean=True + "storage", "force_foolscap", default=True, boolean=True ), handler_overrides=handler_overrides, certFile=certfile, diff --git a/src/allmydata/test/common_system.py b/src/allmydata/test/common_system.py index 0c7d7f747..d49e7831d 100644 --- a/src/allmydata/test/common_system.py +++ b/src/allmydata/test/common_system.py @@ -814,7 +814,7 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): sethelper = partial(setconf, config, which, "helper") setnode("nickname", u"client %d \N{BLACK SMILING FACE}" % (which,)) - setnode("force_foolscap", str(force_foolscap)) + setconf(config, which, "storage", "force_foolscap", str(force_foolscap)) tub_location_hint, tub_port_endpoint = self.port_assigner.assign(reactor) setnode("tub.port", tub_port_endpoint) From 2a5e8e59715ec647387f77b83733d9541886544b Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 15 Nov 2022 15:02:15 -0500 Subject: [PATCH 40/47] Better cleanup. --- src/allmydata/storage_client.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 59d3406f1..8e9ad3656 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -970,6 +970,7 @@ class HTTPNativeStorageServer(service.MultiService): self._connection_status = connection_status.ConnectionStatus.unstarted() self._version = None self._last_connect_time = None + self._connecting_deferred = None def get_permutation_seed(self): return self._permutation_seed @@ -1060,20 +1061,30 @@ class HTTPNativeStorageServer(service.MultiService): def stop_connecting(self): self._lc.stop() + if self._connecting_deferred is not None: + self._connecting_deferred.cancel() def try_to_connect(self): self._connect() def _connect(self): result = self._istorage_server.get_version() + + def remove_connecting_deferred(result): + self._connecting_deferred = None + return result + # Set a short timeout since we're relying on this for server liveness. - result.addTimeout(5, self._reactor) - result.addCallbacks( + self._connecting_deferred = result.addTimeout(5, self._reactor).addBoth( + remove_connecting_deferred).addCallbacks( self._got_version, self._failed_to_connect ) def stopService(self): + if self._connecting_deferred is not None: + self._connecting_deferred.cancel() + result = service.MultiService.stopService(self) if self._lc.running: self._lc.stop() From a20943e10c7d1f4b30b383138f489e9c9dd1eb85 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 16 Nov 2022 09:33:01 -0500 Subject: [PATCH 41/47] As an experiment, see if this fixes failing CI. --- src/allmydata/test/common_system.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/test/common_system.py b/src/allmydata/test/common_system.py index d49e7831d..8bc25aacf 100644 --- a/src/allmydata/test/common_system.py +++ b/src/allmydata/test/common_system.py @@ -649,7 +649,7 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): def setUp(self): self._http_client_pools = [] http_client.StorageClient.start_test_mode(self._http_client_pools.append) - self.addCleanup(http_client.StorageClient.stop_test_mode) + #self.addCleanup(http_client.StorageClient.stop_test_mode) self.port_assigner = SameProcessStreamEndpointAssigner() self.port_assigner.setUp() self.addCleanup(self.port_assigner.tearDown) From 9f5f287473d734932f348d77b89fb81838e5c3d1 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 16 Nov 2022 09:57:39 -0500 Subject: [PATCH 42/47] Nope, not helpful. --- src/allmydata/test/common_system.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/test/common_system.py b/src/allmydata/test/common_system.py index 8bc25aacf..d49e7831d 100644 --- a/src/allmydata/test/common_system.py +++ b/src/allmydata/test/common_system.py @@ -649,7 +649,7 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): def setUp(self): self._http_client_pools = [] http_client.StorageClient.start_test_mode(self._http_client_pools.append) - #self.addCleanup(http_client.StorageClient.stop_test_mode) + self.addCleanup(http_client.StorageClient.stop_test_mode) self.port_assigner = SameProcessStreamEndpointAssigner() self.port_assigner.setUp() self.addCleanup(self.port_assigner.tearDown) From 2ab172ffca9c6faac1751709ce5db5d17e4e28db Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 16 Nov 2022 10:26:29 -0500 Subject: [PATCH 43/47] Try to set more aggressive timeouts when testing. --- src/allmydata/test/common_system.py | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/allmydata/test/common_system.py b/src/allmydata/test/common_system.py index d49e7831d..e75021248 100644 --- a/src/allmydata/test/common_system.py +++ b/src/allmydata/test/common_system.py @@ -648,7 +648,7 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): def setUp(self): self._http_client_pools = [] - http_client.StorageClient.start_test_mode(self._http_client_pools.append) + http_client.StorageClient.start_test_mode(self._got_new_http_connection_pool) self.addCleanup(http_client.StorageClient.stop_test_mode) self.port_assigner = SameProcessStreamEndpointAssigner() self.port_assigner.setUp() @@ -657,6 +657,23 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): self.sparent = service.MultiService() self.sparent.startService() + def _got_new_http_connection_pool(self, pool): + # Register the pool for shutdown later: + self._http_client_pools.append(pool) + # Disable retries: + pool.retryAutomatically = False + # Make a much more aggressive timeout for connections, we're connecting + # locally after all... and also make sure it's lower than the delay we + # add in tearDown, to prevent dirty reactor issues. + getConnection = pool.getConnection + + def getConnectionWithTimeout(*args, **kwargs): + d = getConnection(*args, **kwargs) + d.addTimeout(0.05, reactor) + return d + + pool.getConnection = getConnectionWithTimeout + def close_idle_http_connections(self): """Close all HTTP client connections that are just hanging around.""" return defer.gatherResults( @@ -668,7 +685,7 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): d = self.sparent.stopService() d.addBoth(flush_but_dont_ignore) d.addBoth(lambda x: self.close_idle_http_connections().addCallback(lambda _: x)) - d.addBoth(lambda x: deferLater(reactor, 0.02, lambda: x)) + d.addBoth(lambda x: deferLater(reactor, 0.1, lambda: x)) return d def getdir(self, subdir): From 35317373474c5170d9a15b5b9cd895ceb7222391 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 16 Nov 2022 10:36:11 -0500 Subject: [PATCH 44/47] Make timeouts less aggressive, CI machines are slow? --- src/allmydata/test/common_system.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/allmydata/test/common_system.py b/src/allmydata/test/common_system.py index e75021248..8d3019935 100644 --- a/src/allmydata/test/common_system.py +++ b/src/allmydata/test/common_system.py @@ -669,7 +669,7 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): def getConnectionWithTimeout(*args, **kwargs): d = getConnection(*args, **kwargs) - d.addTimeout(0.05, reactor) + d.addTimeout(1, reactor) return d pool.getConnection = getConnectionWithTimeout @@ -685,7 +685,7 @@ class SystemTestMixin(pollmixin.PollMixin, testutil.StallMixin): d = self.sparent.stopService() d.addBoth(flush_but_dont_ignore) d.addBoth(lambda x: self.close_idle_http_connections().addCallback(lambda _: x)) - d.addBoth(lambda x: deferLater(reactor, 0.1, lambda: x)) + d.addBoth(lambda x: deferLater(reactor, 2, lambda: x)) return d def getdir(self, subdir): From 097d918a240ba291ebd6b00108f071362eefcbd3 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 16 Nov 2022 13:37:50 -0500 Subject: [PATCH 45/47] Sigh --- src/allmydata/test/test_storage_https.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/allmydata/test/test_storage_https.py b/src/allmydata/test/test_storage_https.py index bacb40290..a9421c3e5 100644 --- a/src/allmydata/test/test_storage_https.py +++ b/src/allmydata/test/test_storage_https.py @@ -179,6 +179,10 @@ class PinningHTTPSValidation(AsyncTestCase): response = await self.request(url, certificate) self.assertEqual(await response.content(), b"YOYODYNE") + # We keep getting TLSMemoryBIOProtocol being left around, so try harder + # to wait for it to finish. + await deferLater(reactor, 0.01) + @async_to_deferred async def test_server_certificate_not_valid_yet(self): """ From d182a2f1865002cc9a3167c1f585413ac6db4307 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 17 Nov 2022 11:01:12 -0500 Subject: [PATCH 46/47] Add the delay to appropriate test. --- src/allmydata/test/test_storage_https.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/allmydata/test/test_storage_https.py b/src/allmydata/test/test_storage_https.py index a9421c3e5..01431267f 100644 --- a/src/allmydata/test/test_storage_https.py +++ b/src/allmydata/test/test_storage_https.py @@ -144,6 +144,10 @@ class PinningHTTPSValidation(AsyncTestCase): response = await self.request(url, certificate) self.assertEqual(await response.content(), b"YOYODYNE") + # We keep getting TLSMemoryBIOProtocol being left around, so try harder + # to wait for it to finish. + await deferLater(reactor, 0.01) + @async_to_deferred async def test_server_certificate_has_wrong_hash(self): """ @@ -179,10 +183,6 @@ class PinningHTTPSValidation(AsyncTestCase): response = await self.request(url, certificate) self.assertEqual(await response.content(), b"YOYODYNE") - # We keep getting TLSMemoryBIOProtocol being left around, so try harder - # to wait for it to finish. - await deferLater(reactor, 0.01) - @async_to_deferred async def test_server_certificate_not_valid_yet(self): """ From 9b21f1da90a1b80414959822fec689040db75d40 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 17 Nov 2022 11:35:10 -0500 Subject: [PATCH 47/47] Increase how many statuses are stored. --- src/allmydata/history.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/allmydata/history.py b/src/allmydata/history.py index b5cfb7318..06a22ab5d 100644 --- a/src/allmydata/history.py +++ b/src/allmydata/history.py @@ -20,7 +20,7 @@ class History(object): MAX_UPLOAD_STATUSES = 10 MAX_MAPUPDATE_STATUSES = 20 MAX_PUBLISH_STATUSES = 20 - MAX_RETRIEVE_STATUSES = 20 + MAX_RETRIEVE_STATUSES = 40 def __init__(self, stats_provider=None): self.stats_provider = stats_provider