From 5f196050753b70c9224f1a10427d13649553b66a Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Mon, 1 May 2023 11:41:51 -0400 Subject: [PATCH 01/15] During testing, ensure we're not getting text/html unexpectedly. --- src/allmydata/storage/http_client.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index f786b8f30..7314adf38 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -443,11 +443,20 @@ class StorageClient(object): kwargs["data"] = dumps(message_to_serialize) headers.addRawHeader("Content-Type", CBOR_MIME_TYPE) - return await self._treq.request( + response = await self._treq.request( method, url, headers=headers, timeout=timeout, **kwargs ) - async def decode_cbor(self, response, schema: Schema) -> object: + if self.TEST_MODE_REGISTER_HTTP_POOL is not None: + if response.code != 404: + # We're doing API queries, HTML is never correct except in 404, but + # it's the default for Twisted's web server so make sure nothing + # unexpected happened. + assert get_content_type(response.headers) != "text/html" + + return response + + async def decode_cbor(self, response: IResponse, schema: Schema) -> object: """Given HTTP response, return decoded CBOR body.""" with start_action(action_type="allmydata:storage:http-client:decode-cbor"): if response.code > 199 and response.code < 300: From fbd6dbda47fec79f016ff5ba4609f1b03203a248 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Mon, 1 May 2023 11:42:02 -0400 Subject: [PATCH 02/15] text/html is a bad default content type. --- src/allmydata/storage/http_server.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py index 8647274f8..e0040d377 100644 --- a/src/allmydata/storage/http_server.py +++ b/src/allmydata/storage/http_server.py @@ -106,6 +106,9 @@ def _authorization_decorator(required_secrets): def decorator(f): @wraps(f) def route(self, request, *args, **kwargs): + # Don't set text/html content type by default: + request.defaultContentType = None + with start_action( action_type="allmydata:storage:http-server:handle-request", method=request.method, From 2292d64fcddc5d585551a0310a6b3076eb68caf3 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Mon, 1 May 2023 11:49:09 -0400 Subject: [PATCH 03/15] Set a better content type for data downloads. --- src/allmydata/storage/http_client.py | 6 ++++++ src/allmydata/storage/http_server.py | 4 +++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/allmydata/storage/http_client.py b/src/allmydata/storage/http_client.py index 7314adf38..64962e7b6 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -596,6 +596,12 @@ def read_share_chunk( if response.code == http.NO_CONTENT: return b"" + content_type = get_content_type(response.headers) + if content_type != "application/octet-stream": + raise ValueError( + f"Content-type was wrong: {content_type}, should be application/octet-stream" + ) + if response.code == http.PARTIAL_CONTENT: content_range = parse_content_range_header( response.headers.getRawHeaders("content-range")[0] or "" diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py index e0040d377..0791c3389 100644 --- a/src/allmydata/storage/http_server.py +++ b/src/allmydata/storage/http_server.py @@ -778,6 +778,7 @@ class HTTPServer(object): ) def read_share_chunk(self, request, authorization, storage_index, share_number): """Read a chunk for an already uploaded immutable.""" + request.setHeader("content-type", "application/octet-stream") try: bucket = self._storage_server.get_buckets(storage_index)[share_number] except KeyError: @@ -883,7 +884,8 @@ class HTTPServer(object): ) def read_mutable_chunk(self, request, authorization, storage_index, share_number): """Read a chunk from a mutable.""" - + request.setHeader("content-type", "application/octet-stream") + try: share_length = self._storage_server.get_mutable_share_length( storage_index, share_number From 5632e82e1338541d3c8f574070f402fabdc3523c Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Mon, 1 May 2023 11:49:29 -0400 Subject: [PATCH 04/15] News fragment. --- newsfragments/4016.minor | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 newsfragments/4016.minor diff --git a/newsfragments/4016.minor b/newsfragments/4016.minor new file mode 100644 index 000000000..e69de29bb From 8c8e24a3b9c7655c197d24ece14e559511727610 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Mon, 1 May 2023 11:50:05 -0400 Subject: [PATCH 05/15] Black reformat. --- src/allmydata/storage/http_server.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/allmydata/storage/http_server.py b/src/allmydata/storage/http_server.py index 0791c3389..7d7398b1e 100644 --- a/src/allmydata/storage/http_server.py +++ b/src/allmydata/storage/http_server.py @@ -117,9 +117,9 @@ def _authorization_decorator(required_secrets): try: # Check Authorization header: if not timing_safe_compare( - request.requestHeaders.getRawHeaders("Authorization", [""])[0].encode( - "utf-8" - ), + request.requestHeaders.getRawHeaders("Authorization", [""])[ + 0 + ].encode("utf-8"), swissnum_auth_header(self._swissnum), ): raise _HTTPError(http.UNAUTHORIZED) @@ -494,6 +494,7 @@ def read_range( def _add_error_handling(app: Klein): """Add exception handlers to a Klein app.""" + @app.handle_errors(_HTTPError) def _http_error(_, request, failure): """Handle ``_HTTPError`` exceptions.""" @@ -885,7 +886,7 @@ class HTTPServer(object): def read_mutable_chunk(self, request, authorization, storage_index, share_number): """Read a chunk from a mutable.""" request.setHeader("content-type", "application/octet-stream") - + try: share_length = self._storage_server.get_mutable_share_length( storage_index, share_number From 19690c9c7bd319bf9a2d1931ef4c8b37e9cc3803 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 4 May 2023 13:05:06 -0400 Subject: [PATCH 06/15] Don't mix blocking and async APIs! --- integration/test_web.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/integration/test_web.py b/integration/test_web.py index b3c4a8e5f..c7d2275ae 100644 --- a/integration/test_web.py +++ b/integration/test_web.py @@ -14,6 +14,8 @@ from __future__ import annotations import time from urllib.parse import unquote as url_unquote, quote as url_quote +from twisted.internet.defer import maybeDeferred + import allmydata.uri from allmydata.util import jsonbytes as json @@ -24,7 +26,7 @@ import requests import html5lib from bs4 import BeautifulSoup -from pytest_twisted import ensureDeferred +import pytest_twisted @run_in_thread def test_index(alice): @@ -185,7 +187,7 @@ def test_deep_stats(alice): time.sleep(.5) -@util.run_in_thread +@run_in_thread def test_status(alice): """ confirm we get something sensible from /status and the various sub-types @@ -251,7 +253,7 @@ def test_status(alice): assert found_download, "Failed to find the file we downloaded in the status-page" -@ensureDeferred +@run_in_thread async def test_directory_deep_check(reactor, request, alice): """ use deep-check and confirm the result pages work @@ -262,7 +264,8 @@ async def test_directory_deep_check(reactor, request, alice): required = 2 total = 4 - await util.reconfigure(reactor, request, alice, (happy, required, total), convergence=None) + result = util.reconfigure(reactor, request, alice, (happy, required, total), convergence=None) + pytest_twisted.blockon(maybeDeferred(result)) # create a directory resp = requests.post( From f54a2d3d76a0508c713c42c420105f98e384a5df Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 4 May 2023 13:05:46 -0400 Subject: [PATCH 07/15] News file. --- newsfragments/4023.minor | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 newsfragments/4023.minor diff --git a/newsfragments/4023.minor b/newsfragments/4023.minor new file mode 100644 index 000000000..e69de29bb From 3d6b3b3b74f2fc4878629e9838471f3d12a1eb6b Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 4 May 2023 13:10:23 -0400 Subject: [PATCH 08/15] Use modern Docker image. --- .readthedocs.yaml | 5 +++++ newsfragments/4026.minor | 0 2 files changed, 5 insertions(+) create mode 100644 newsfragments/4026.minor diff --git a/.readthedocs.yaml b/.readthedocs.yaml index 65b390f26..665b53178 100644 --- a/.readthedocs.yaml +++ b/.readthedocs.yaml @@ -1,5 +1,10 @@ version: 2 +build: + os: ubuntu-22.04 + tools: + python: "3.10" + python: install: - requirements: docs/requirements.txt diff --git a/newsfragments/4026.minor b/newsfragments/4026.minor new file mode 100644 index 000000000..e69de29bb From bee295e4119448958f1ea2b07666345328eaaee9 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 4 May 2023 13:33:54 -0400 Subject: [PATCH 09/15] Actually run the test. --- integration/test_web.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/integration/test_web.py b/integration/test_web.py index c7d2275ae..1d9498264 100644 --- a/integration/test_web.py +++ b/integration/test_web.py @@ -14,7 +14,7 @@ from __future__ import annotations import time from urllib.parse import unquote as url_unquote, quote as url_quote -from twisted.internet.defer import maybeDeferred +from twisted.internet.defer import ensureDeferred import allmydata.uri from allmydata.util import jsonbytes as json @@ -254,7 +254,7 @@ def test_status(alice): @run_in_thread -async def test_directory_deep_check(reactor, request, alice): +def test_directory_deep_check(reactor, request, alice): """ use deep-check and confirm the result pages work """ @@ -265,7 +265,7 @@ async def test_directory_deep_check(reactor, request, alice): total = 4 result = util.reconfigure(reactor, request, alice, (happy, required, total), convergence=None) - pytest_twisted.blockon(maybeDeferred(result)) + pytest_twisted.blockon(ensureDeferred(result)) # create a directory resp = requests.post( From 3b52457d1c42e4de5bffa7486738a837d81ceb56 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 4 May 2023 13:54:04 -0400 Subject: [PATCH 10/15] Try a different way. --- integration/test_web.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/integration/test_web.py b/integration/test_web.py index 1d9498264..fd29504f8 100644 --- a/integration/test_web.py +++ b/integration/test_web.py @@ -14,7 +14,7 @@ from __future__ import annotations import time from urllib.parse import unquote as url_unquote, quote as url_quote -from twisted.internet.defer import ensureDeferred +from twisted.internet.threads import deferToThread import allmydata.uri from allmydata.util import jsonbytes as json @@ -253,8 +253,8 @@ def test_status(alice): assert found_download, "Failed to find the file we downloaded in the status-page" -@run_in_thread -def test_directory_deep_check(reactor, request, alice): +@pytest_twisted.ensureDeferred +async def test_directory_deep_check(reactor, request, alice): """ use deep-check and confirm the result pages work """ @@ -264,9 +264,11 @@ def test_directory_deep_check(reactor, request, alice): required = 2 total = 4 - result = util.reconfigure(reactor, request, alice, (happy, required, total), convergence=None) - pytest_twisted.blockon(ensureDeferred(result)) + await util.reconfigure(reactor, request, alice, (happy, required, total), convergence=None) + await deferToThread(_test_directory_deep_check_blocking, alice) + +def _test_directory_deep_check_blocking(alice): # create a directory resp = requests.post( util.node_url(alice.node_dir, u"uri"), From 049502e8c282eb507d46cdfcf24a945aceaa7e33 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 4 May 2023 16:15:30 -0400 Subject: [PATCH 11/15] Don't mix blocking and async code. --- integration/test_get_put.py | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/integration/test_get_put.py b/integration/test_get_put.py index f121d6284..0aca6954a 100644 --- a/integration/test_get_put.py +++ b/integration/test_get_put.py @@ -6,8 +6,10 @@ and stdout. from subprocess import Popen, PIPE, check_output, check_call import pytest -from pytest_twisted import ensureDeferred +from pytest_twisted import blockon from twisted.internet import reactor +from twisted.internet.threads import blockingCallFromThread +from twisted.internet.defer import Deferred from .util import run_in_thread, cli, reconfigure @@ -86,8 +88,8 @@ def test_large_file(alice, get_put_alias, tmp_path): assert outfile.read_bytes() == tempfile.read_bytes() -@ensureDeferred -async def test_upload_download_immutable_different_default_max_segment_size(alice, get_put_alias, tmpdir, request): +@run_in_thread +def test_upload_download_immutable_different_default_max_segment_size(alice, get_put_alias, tmpdir, request): """ Tahoe-LAFS used to have a default max segment size of 128KB, and is now 1MB. Test that an upload created when 128KB was the default can be @@ -100,22 +102,25 @@ async def test_upload_download_immutable_different_default_max_segment_size(alic with tempfile.open("wb") as f: f.write(large_data) - async def set_segment_size(segment_size): - await reconfigure( + def set_segment_size(segment_size): + return blockingCallFromThread( reactor, - request, - alice, - (1, 1, 1), - None, - max_segment_size=segment_size - ) + lambda: Deferred.fromCoroutine(reconfigure( + reactor, + request, + alice, + (1, 1, 1), + None, + max_segment_size=segment_size + )) + ) # 1. Upload file 1 with default segment size set to 1MB - await set_segment_size(1024 * 1024) + set_segment_size(1024 * 1024) cli(alice, "put", str(tempfile), "getput:seg1024kb") # 2. Download file 1 with default segment size set to 128KB - await set_segment_size(128 * 1024) + set_segment_size(128 * 1024) assert large_data == check_output( ["tahoe", "--node-directory", alice.node_dir, "get", "getput:seg1024kb", "-"] ) @@ -124,7 +129,7 @@ async def test_upload_download_immutable_different_default_max_segment_size(alic cli(alice, "put", str(tempfile), "getput:seg128kb") # 4. Download file 2 with default segment size set to 1MB - await set_segment_size(1024 * 1024) + set_segment_size(1024 * 1024) assert large_data == check_output( ["tahoe", "--node-directory", alice.node_dir, "get", "getput:seg128kb", "-"] ) From ba638f9ff602b3f7dcf8290f58adbab653ba25bd Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 4 May 2023 17:08:54 -0400 Subject: [PATCH 12/15] Remove unnecessary import. --- integration/test_get_put.py | 1 - 1 file changed, 1 deletion(-) diff --git a/integration/test_get_put.py b/integration/test_get_put.py index 0aca6954a..e30a34f97 100644 --- a/integration/test_get_put.py +++ b/integration/test_get_put.py @@ -6,7 +6,6 @@ and stdout. from subprocess import Popen, PIPE, check_output, check_call import pytest -from pytest_twisted import blockon from twisted.internet import reactor from twisted.internet.threads import blockingCallFromThread from twisted.internet.defer import Deferred From 20f55933bdf4742da8b0e68f9cd14fa0522a6323 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 5 May 2023 11:02:24 -0400 Subject: [PATCH 13/15] Update past deprecated runner. --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fe911e34d..77221e552 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -248,7 +248,7 @@ jobs: fail-fast: false matrix: os: - - macos-10.15 + - macos-latest - windows-latest - ubuntu-latest python-version: From 2e22df60fe6b29706464d3d3fa4f8b2e7baf51d5 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Mon, 8 May 2023 13:33:34 -0400 Subject: [PATCH 14/15] Try with fewer persistent HTTP connections. --- 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 64962e7b6..0e12df7ce 100644 --- a/src/allmydata/storage/http_client.py +++ b/src/allmydata/storage/http_client.py @@ -325,7 +325,7 @@ class StorageClient(object): certificate_hash = nurl.user.encode("ascii") if pool is None: pool = HTTPConnectionPool(reactor) - pool.maxPersistentPerHost = 20 + pool.maxPersistentPerHost = 10 if cls.TEST_MODE_REGISTER_HTTP_POOL is not None: cls.TEST_MODE_REGISTER_HTTP_POOL(pool) From 2bd76058e927c7811b722538abb8127fadde45be Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Mon, 8 May 2023 13:48:54 -0400 Subject: [PATCH 15/15] Specific version of macOS. --- .github/workflows/ci.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 77221e552..dc9854ae4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -53,9 +53,9 @@ jobs: - "3.11" include: # On macOS don't bother with 3.8, just to get faster builds. - - os: macos-latest + - os: macos-12 python-version: "3.9" - - os: macos-latest + - os: macos-12 python-version: "3.11" # We only support PyPy on Linux at the moment. - os: ubuntu-latest @@ -165,7 +165,7 @@ jobs: fail-fast: false matrix: include: - - os: macos-latest + - os: macos-12 python-version: "3.9" force-foolscap: false - os: windows-latest @@ -248,7 +248,7 @@ jobs: fail-fast: false matrix: os: - - macos-latest + - macos-12 - windows-latest - ubuntu-latest python-version: