From c92c93e6d56975c47bd8b2e0dea3a8cfba81960b Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 10 May 2023 16:31:53 -0400 Subject: [PATCH 1/5] Clean up cached HTTP connections on shutdown. --- newsfragments/4028.minor | 0 src/allmydata/storage/http_client.py | 7 ++++++- src/allmydata/storage_client.py | 5 +++++ 3 files changed, 11 insertions(+), 1 deletion(-) create mode 100644 newsfragments/4028.minor diff --git a/newsfragments/4028.minor b/newsfragments/4028.minor new file mode 100644 index 000000000..e69de29bb diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index 0e12df7ce..e2b45e30c 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -310,6 +310,7 @@ class StorageClient(object): _base_url: DecodedURL _swissnum: bytes _treq: Union[treq, StubTreq, HTTPClient] + _pool: HTTPConnectionPool _clock: IReactorTime @classmethod @@ -339,7 +340,7 @@ class StorageClient(object): ) https_url = DecodedURL().replace(scheme="https", host=nurl.host, port=nurl.port) - return cls(https_url, swissnum, treq_client, reactor) + return cls(https_url, swissnum, treq_client, pool, reactor) def relative_url(self, path: str) -> DecodedURL: """Get a URL relative to the base URL.""" @@ -479,6 +480,10 @@ class StorageClient(object): ).read() raise ClientException(response.code, response.phrase, data) + def shutdown(self) -> Deferred: + """Shutdown any connections.""" + return self._pool.closeCachedConnections() + @define(hash=True) class StorageClientGeneral(object): diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index a40e98b03..94aae43f6 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -1271,6 +1271,11 @@ class HTTPNativeStorageServer(service.MultiService): if self._lc.running: self._lc.stop() self._failed_to_connect("shut down") + + maybe_storage_server = self.get_storage_server() + if maybe_storage_server is not None: + result.addCallback(lambda _: maybe_storage_server._http_client.shutdown()) + return result From ba9946e6ea863b47f5b6a544cf6b70db49dd4bf6 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 10 May 2023 16:34:02 -0400 Subject: [PATCH 2/5] Fix tests. --- src/allmydata/storage/http_client.py | 5 +++-- src/allmydata/test/test_storage_http.py | 5 ++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index e2b45e30c..9c4a5538c 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -310,7 +310,7 @@ class StorageClient(object): _base_url: DecodedURL _swissnum: bytes _treq: Union[treq, StubTreq, HTTPClient] - _pool: HTTPConnectionPool + _pool: Optional[HTTPConnectionPool] _clock: IReactorTime @classmethod @@ -482,7 +482,8 @@ class StorageClient(object): def shutdown(self) -> Deferred: """Shutdown any connections.""" - return self._pool.closeCachedConnections() + if self._pool is not None: + return self._pool.closeCachedConnections() @define(hash=True) diff --git a/src/allmydata/test/test_storage_http.py b/src/allmydata/test/test_storage_http.py index eca2be1c1..64491f7ae 100644 --- a/src/allmydata/test/test_storage_http.py +++ b/src/allmydata/test/test_storage_http.py @@ -331,6 +331,7 @@ class CustomHTTPServerTests(SyncTestCase): DecodedURL.from_text("http://127.0.0.1"), SWISSNUM_FOR_TEST, treq=treq, + pool=None, # We're using a Treq private API to get the reactor, alas, but only # in a test, so not going to worry about it too much. This would be # fixed if https://github.com/twisted/treq/issues/226 were ever @@ -512,6 +513,7 @@ class HttpTestFixture(Fixture): DecodedURL.from_text("http://127.0.0.1"), SWISSNUM_FOR_TEST, treq=self.treq, + pool=None, clock=self.clock, ) @@ -624,6 +626,7 @@ class GenericHTTPAPITests(SyncTestCase): DecodedURL.from_text("http://127.0.0.1"), b"something wrong", treq=StubTreq(self.http.http_server.get_resource()), + pool=None, clock=self.http.clock, ) ) @@ -1455,7 +1458,7 @@ class SharedImmutableMutableTestsMixin: self.client.advise_corrupt_share(storage_index, 13, reason) ) - for (si, share_number) in [(storage_index, 11), (urandom(16), 13)]: + for si, share_number in [(storage_index, 11), (urandom(16), 13)]: with assert_fails_with_http_code(self, http.NOT_FOUND): self.http.result_of_with_flush( self.client.advise_corrupt_share(si, share_number, reason) From 2ec1c1e43e0bae315897e9d6bb0d7e2df2640cb0 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 10 May 2023 17:23:15 -0400 Subject: [PATCH 3/5] Shut down alice. --- integration/conftest.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/integration/conftest.py b/integration/conftest.py index bf04e4424..6892b33a7 100644 --- a/integration/conftest.py +++ b/integration/conftest.py @@ -401,9 +401,6 @@ def alice( reactor, request, temp_dir, introducer_furl, flog_gatherer, "alice", web_port="tcp:9980:interface=localhost", storage=False, - # We're going to kill this ourselves, so no need for finalizer to - # do it: - finalize=False, ) ) pytest_twisted.blockon(await_client_ready(process)) From d15ea8cb52b87426c82fd1c70581ced87c7d391e Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 23 May 2023 13:24:29 -0400 Subject: [PATCH 4/5] Shutdown more immediately. --- 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 94aae43f6..8541bb40c 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -1274,7 +1274,8 @@ class HTTPNativeStorageServer(service.MultiService): maybe_storage_server = self.get_storage_server() if maybe_storage_server is not None: - result.addCallback(lambda _: maybe_storage_server._http_client.shutdown()) + client_shutting_down = maybe_storage_server._http_client.shutdown() + result.addCallback(lambda _: client_shutting_down) return result From 1e46e36ee2cde0872b6fd0237f3586c489af0352 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 23 May 2023 13:46:32 -0400 Subject: [PATCH 5/5] More direct approach. --- src/allmydata/storage_client.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/allmydata/storage_client.py b/src/allmydata/storage_client.py index 8541bb40c..200f693c0 100644 --- a/src/allmydata/storage_client.py +++ b/src/allmydata/storage_client.py @@ -1272,9 +1272,8 @@ class HTTPNativeStorageServer(service.MultiService): self._lc.stop() self._failed_to_connect("shut down") - maybe_storage_server = self.get_storage_server() - if maybe_storage_server is not None: - client_shutting_down = maybe_storage_server._http_client.shutdown() + if self._istorage_server is not None: + client_shutting_down = self._istorage_server._http_client.shutdown() result.addCallback(lambda _: client_shutting_down) return result