From 262d9d85b97cb064da47c82bab22e62b48db6cd4 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 3 Nov 2022 14:14:21 -0400 Subject: [PATCH] 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