From 27092b2cb703f2910b0a255a1b8250623999e12c Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Tue, 21 Apr 2026 18:27:16 +0500 Subject: [PATCH] Small download handler fixes and improvements (#7441) * Popen cleanup. * Improve the test for response headers. * Fix default headers of Httpx and H2 handlers, add tests. * Check Location in redirect tests. * Simplify bindaddress tests. * Add download_latency to HttpxDownloadHandler, add tests. * Remove explicit method="GET". * Don't expect a specific value in test_download_with_maxsize_very_large_file * Use a non-existent scheme in test_unsupported_scheme. * Remporarily rollback the test_download_latency test check. * Better check in test_download_latency. * Improve logic for StreamCloseReason.CANCELLED. --- scrapy/core/downloader/handlers/_httpx.py | 7 +- scrapy/core/http2/protocol.py | 3 +- scrapy/core/http2/stream.py | 17 ++- tests/mockserver/dns.py | 5 +- tests/mockserver/http_base.py | 7 +- tests/mockserver/http_resources.py | 2 +- tests/test_downloader_handler_httpx.py | 16 +-- .../test_downloader_handler_twisted_http10.py | 4 +- .../test_downloader_handler_twisted_http2.py | 4 +- tests/test_downloader_handlers_http_base.py | 118 ++++++++++++++---- tests/test_proxy_connect.py | 36 +++--- 11 files changed, 144 insertions(+), 75 deletions(-) diff --git a/scrapy/core/downloader/handlers/_httpx.py b/scrapy/core/downloader/handlers/_httpx.py index 83009c0ed..313fb1c4f 100644 --- a/scrapy/core/downloader/handlers/_httpx.py +++ b/scrapy/core/downloader/handlers/_httpx.py @@ -5,6 +5,7 @@ from __future__ import annotations import ipaddress import logging import ssl +import time from http.cookiejar import Cookie, CookieJar from io import BytesIO from typing import TYPE_CHECKING, Any, NoReturn, TypedDict @@ -117,6 +118,9 @@ class HttpxDownloadHandler(BaseHttpDownloadHandler): local_address=self._bind_address, ), ) + # https://github.com/encode/httpx/discussions/1566 + for header_name in ("accept", "accept-encoding", "user-agent"): + self._client.headers.pop(header_name, None) async def download_request(self, request: Request) -> Response: self._warn_unsupported_meta(request.meta) @@ -124,9 +128,10 @@ class HttpxDownloadHandler(BaseHttpDownloadHandler): timeout: float = request.meta.get( "download_timeout", self._DEFAULT_CONNECT_TIMEOUT ) - + start_time = time.monotonic() try: async with self._get_httpx_response(request, timeout) as httpx_response: + request.meta["download_latency"] = time.monotonic() - start_time return await self._read_response(httpx_response, request) except httpx.TimeoutException as e: raise DownloadTimeoutError( diff --git a/scrapy/core/http2/protocol.py b/scrapy/core/http2/protocol.py index 173545f55..39703f976 100644 --- a/scrapy/core/http2/protocol.py +++ b/scrapy/core/http2/protocol.py @@ -40,7 +40,6 @@ from scrapy.utils.ssl import _log_ssl_conn_debug_info if TYPE_CHECKING: from ipaddress import IPv4Address, IPv6Address - from hpack import HeaderTuple from twisted.internet.defer import Deferred from twisted.python.failure import Failure from twisted.web.client import URI @@ -419,7 +418,7 @@ class H2ClientProtocol(Protocol, TimeoutMixin): except KeyError: pass # We ignore server-initiated events else: - stream.receive_headers(cast("list[HeaderTuple]", event.headers)) + stream.receive_headers(cast("list[tuple[str, str]]", event.headers)) def settings_acknowledged(self, event: SettingsAcknowledged) -> None: self.metadata["settings_acknowledged"] = True diff --git a/scrapy/core/http2/stream.py b/scrapy/core/http2/stream.py index 21ce4942e..71569b217 100644 --- a/scrapy/core/http2/stream.py +++ b/scrapy/core/http2/stream.py @@ -24,8 +24,6 @@ from scrapy.utils.httpobj import urlparse_cached if TYPE_CHECKING: from collections.abc import Sequence - from hpack import HeaderTuple - from scrapy.core.http2.protocol import H2ClientProtocol from scrapy.http import Request, Response @@ -153,6 +151,8 @@ class Stream: "flow_controlled_size": 0, # Headers received after sending the request "headers": Headers(), + # Response status code + "status": None, } def _cancel(_: Any) -> None: @@ -361,9 +361,13 @@ class Stream: self._response["flow_controlled_size"], self.stream_id ) - def receive_headers(self, headers: list[HeaderTuple]) -> None: + def receive_headers(self, headers: list[tuple[str, str]]) -> None: for name, value in headers: - self._response["headers"].appendlist(name, value) + if name == ":status": + # it's a pseudo-header + self._response["status"] = int(value) + else: + self._response["headers"].appendlist(name, value) # Check if we exceed the allowed max data size which can be received expected_size = int(self._response["headers"].get(b"Content-Length", -1)) @@ -453,7 +457,8 @@ class Stream: # There maybe no :status in headers, we make # HTTP Status Code: 499 - Client Closed Request - self._response["headers"][":status"] = "499" + if self._response["status"] is None: + self._response["status"] = 499 self._fire_response_deferred() elif reason is StreamCloseReason.RESET: @@ -492,7 +497,7 @@ class Stream: response = make_response( url=self._request.url, - status=int(self._response["headers"][":status"]), + status=self._response["status"], headers=self._response["headers"], body=self._response["body"].getvalue(), certificate=self._protocol.metadata["certificate"], diff --git a/tests/mockserver/dns.py b/tests/mockserver/dns.py index 7c08f237c..e19a2e61a 100644 --- a/tests/mockserver/dns.py +++ b/tests/mockserver/dns.py @@ -35,11 +35,10 @@ class MockDNSServer: [sys.executable, "-u", "-m", "tests.mockserver.dns"], stdout=PIPE, env=get_script_run_env(), + text=True, ) self.host = "127.0.0.1" - self.port = int( - self.proc.stdout.readline().strip().decode("ascii").split(":")[1] - ) + self.port = int(self.proc.stdout.readline().strip().split(":")[1]) return self def __exit__(self, exc_type, exc_value, traceback): diff --git a/tests/mockserver/http_base.py b/tests/mockserver/http_base.py index 440d38fb9..ffd11c2dd 100644 --- a/tests/mockserver/http_base.py +++ b/tests/mockserver/http_base.py @@ -34,7 +34,7 @@ class BaseMockServer(ABC): if not self.listen_http and not self.listen_https: raise ValueError("At least one of listen_http and listen_https must be set") - self.proc: Popen | None = None + self.proc: Popen[str] | None = None self.host: str = "127.0.0.1" self.http_port: int | None = None self.https_port: int | None = None @@ -44,13 +44,14 @@ class BaseMockServer(ABC): [sys.executable, "-u", "-m", self.module_name, *self.get_additional_args()], stdout=PIPE, env=get_script_run_env(), + text=True, ) if self.listen_http: - http_address = self.proc.stdout.readline().strip().decode("ascii") + http_address = self.proc.stdout.readline().strip() http_parsed = urlparse(http_address) self.http_port = http_parsed.port if self.listen_https: - https_address = self.proc.stdout.readline().strip().decode("ascii") + https_address = self.proc.stdout.readline().strip() https_parsed = urlparse(https_address) self.https_port = https_parsed.port return self diff --git a/tests/mockserver/http_resources.py b/tests/mockserver/http_resources.py index 629932f32..7f52f0092 100644 --- a/tests/mockserver/http_resources.py +++ b/tests/mockserver/http_resources.py @@ -348,7 +348,7 @@ class ResponseHeadersResource(resource.Resource): def render(self, request): body = json.loads(request.content.read().decode()) for header_name, header_value in body.items(): - request.responseHeaders.addRawHeader(header_name, header_value) + request.responseHeaders.setRawHeaders(header_name, [header_value]) return json.dumps(body).encode("utf-8") diff --git a/tests/test_downloader_handler_httpx.py b/tests/test_downloader_handler_httpx.py index 8e8e9a037..3b1796af7 100644 --- a/tests/test_downloader_handler_httpx.py +++ b/tests/test_downloader_handler_httpx.py @@ -43,21 +43,8 @@ class HttpxDownloadHandlerMixin: class TestHttp11(HttpxDownloadHandlerMixin, TestHttp11Base): - @coroutine_test - async def test_unsupported_bindaddress( - self, caplog: pytest.LogCaptureFixture, mockserver: MockServer - ) -> None: - meta = {"bindaddress": ("127.0.0.2", 0)} - request = Request(mockserver.url("/text"), meta=meta) - async with self.get_dh() as download_handler: - response = await download_handler.download_request(request) - assert response.body == b"Works" - assert ( - "The 'bindaddress' request meta key is not supported by HttpxDownloadHandler" - in caplog.text - ) + handler_supports_bindaddress_meta = False - # skip macOS tests @pytest.mark.skipif( sys.platform == "darwin", reason="127.0.0.2 is not available on macOS by default", @@ -91,6 +78,7 @@ class TestHttp11(HttpxDownloadHandlerMixin, TestHttp11Base): class TestHttps11(HttpxDownloadHandlerMixin, TestHttps11Base): + handler_supports_bindaddress_meta = False tls_log_message = "SSL connection to 127.0.0.1 using protocol TLSv1.3, cipher" @pytest.mark.skip(reason="The check is Twisted-specific") diff --git a/tests/test_downloader_handler_twisted_http10.py b/tests/test_downloader_handler_twisted_http10.py index a1cd0b7a6..8281337b0 100644 --- a/tests/test_downloader_handler_twisted_http10.py +++ b/tests/test_downloader_handler_twisted_http10.py @@ -34,9 +34,7 @@ class TestHttp10(HTTP10DownloadHandlerMixin, TestHttpBase): @deferred_f_from_coro_f async def test_protocol(self, mockserver: MockServer) -> None: - request = Request( - mockserver.url("/host", is_secure=self.is_secure), method="GET" - ) + request = Request(mockserver.url("/host", is_secure=self.is_secure)) async with self.get_dh() as download_handler: response = await download_handler.download_request(request) assert response.protocol == "HTTP/1.0" diff --git a/tests/test_downloader_handler_twisted_http2.py b/tests/test_downloader_handler_twisted_http2.py index 53f0210ff..e99257b6f 100644 --- a/tests/test_downloader_handler_twisted_http2.py +++ b/tests/test_downloader_handler_twisted_http2.py @@ -62,9 +62,7 @@ class TestHttps2(H2DownloadHandlerMixin, TestHttps11Base): @deferred_f_from_coro_f async def test_protocol(self, mockserver: MockServer) -> None: - request = Request( - mockserver.url("/host", is_secure=self.is_secure), method="GET" - ) + request = Request(mockserver.url("/host", is_secure=self.is_secure)) async with self.get_dh() as download_handler: response = await download_handler.download_request(request) assert response.protocol == "h2" diff --git a/tests/test_downloader_handlers_http_base.py b/tests/test_downloader_handlers_http_base.py index 9a6a77560..59b6d5754 100644 --- a/tests/test_downloader_handlers_http_base.py +++ b/tests/test_downloader_handlers_http_base.py @@ -4,6 +4,7 @@ from __future__ import annotations import gzip import json +import platform import re import sys from abc import ABC, abstractmethod @@ -11,7 +12,7 @@ from contextlib import asynccontextmanager from http import HTTPStatus from ipaddress import IPv4Address from socket import gethostbyname -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING, Any, ClassVar from urllib.parse import urlparse import pytest @@ -55,6 +56,10 @@ if TYPE_CHECKING: class TestHttpBase(ABC): is_secure = False + # whether the handler supports per-request bindaddress + handler_supports_bindaddress_meta = True + # default headers added by the underlying library that cannot be suppressed + always_present_req_headers: ClassVar[frozenset[str]] = frozenset() @property @abstractmethod @@ -75,7 +80,7 @@ class TestHttpBase(ABC): @coroutine_test async def test_unsupported_scheme(self) -> None: - request = Request("ftp://unsupported.scheme") + request = Request("unsupp://unsupported.scheme") async with self.get_dh() as download_handler: with pytest.raises(UnsupportedURLSchemeError): await download_handler.download_request(request) @@ -184,6 +189,26 @@ class TestHttpBase(ABC): assert "headers" in body assert body["headers"]["X-Custom-Header"] == ["foo", "bar"] + @coroutine_test + async def test_server_receives_no_extra_headers( + self, mockserver: MockServer + ) -> None: + """Test that the handler doesn't add headers to the request.""" + request = Request(mockserver.url("/echo", is_secure=self.is_secure)) + async with self.get_dh() as download_handler: + response = await download_handler.download_request(request) + assert response.status == HTTPStatus.OK + body = json.loads(response.body.decode("utf-8")) + assert "headers" in body + received_headers = set(body["headers"].keys()) + allowed_headers = { + "Connection", + "Content-Length", + "Host", + } | self.always_present_req_headers + extra_headers = received_headers - allowed_headers + assert not extra_headers, body["headers"] + @coroutine_test async def test_server_receives_correct_request_body( self, mockserver: MockServer @@ -236,9 +261,32 @@ class TestHttpBase(ABC): assert header_name in response.headers, ( f"Response was missing expected header {header_name}" ) - assert response.headers[header_name] == bytes( - header_value, encoding="utf-8" - ) + assert response.headers.getlist(header_name) == [ + header_value.encode(encoding="utf-8") + ] + + @coroutine_test + async def test_download_no_extra_response_headers( + self, mockserver: MockServer + ) -> None: + """Test that the handler doesn't add headers to the response.""" + request = Request( + mockserver.url("/response-headers", is_secure=self.is_secure), + headers={"content-type": "application/json"}, + body=json.dumps({}), + ) + async with self.get_dh() as download_handler: + response = await download_handler.download_request(request) + assert response.status == 200 + received_headers = set(response.headers.keys()) + allowed_headers = { + b"Content-Length", + b"Content-Type", + b"Date", + b"Server", + } + extra_headers = received_headers - allowed_headers + assert not extra_headers, response.headers @coroutine_test async def test_redirect_status(self, mockserver: MockServer) -> None: @@ -246,6 +294,7 @@ class TestHttpBase(ABC): async with self.get_dh() as download_handler: response = await download_handler.download_request(request) assert response.status == 302 + assert response.headers["Location"] == b"/redirected" @coroutine_test async def test_redirect_status_head(self, mockserver: MockServer) -> None: @@ -255,6 +304,7 @@ class TestHttpBase(ABC): async with self.get_dh() as download_handler: response = await download_handler.download_request(request) assert response.status == 302 + assert response.headers["Location"] == b"/redirected" @coroutine_test async def test_timeout_download_from_spider_nodata_rcvd( @@ -360,9 +410,7 @@ class TestHttpBase(ABC): @coroutine_test async def test_response_header_content_length(self, mockserver: MockServer) -> None: - request = Request( - mockserver.url("/text", is_secure=self.is_secure), method="GET" - ) + request = Request(mockserver.url("/text", is_secure=self.is_secure)) async with self.get_dh() as download_handler: response = await download_handler.download_request(request) assert response.headers[b"content-length"] == b"5" @@ -457,6 +505,20 @@ class TestHttpBase(ABC): assert "Cookie" not in headers assert "cookie" not in headers + @coroutine_test + async def test_download_latency(self, mockserver: MockServer) -> None: + request = Request(mockserver.url("/text", is_secure=self.is_secure)) + async with self.get_dh() as download_handler: + await download_handler.download_request(request) + assert "download_latency" in request.meta + latency = request.meta["download_latency"] + if sys.version_info < (3, 13) and platform.system() == "Windows": + # time.monotonic() resolution is too low here: + # https://docs.python.org/3/whatsnew/3.13.html#time + assert latency >= 0 + else: + assert latency > 0 + class TestHttp11Base(TestHttpBase): """HTTP 1.1 test case""" @@ -510,9 +572,9 @@ class TestHttp11Base(TestHttpBase): async with self.get_dh({"DOWNLOAD_MAXSIZE": 1_500}) as download_handler: with pytest.raises(DownloadCancelledError): await download_handler.download_request(request) - assert ( - "Received 2048 bytes which is larger than download max size (1500)" - in caplog.text + assert re.search( + r"Received \d+ bytes which is larger than download max size \(1500\)", + caplog.text, ) @coroutine_test @@ -665,42 +727,48 @@ class TestHttp11Base(TestHttpBase): @coroutine_test async def test_protocol(self, mockserver: MockServer) -> None: - request = Request( - mockserver.url("/host", is_secure=self.is_secure), method="GET" - ) + request = Request(mockserver.url("/host", is_secure=self.is_secure)) async with self.get_dh() as download_handler: response = await download_handler.download_request(request) assert response.protocol == "HTTP/1.1" - # skip macOS tests @pytest.mark.skipif( sys.platform == "darwin", reason="127.0.0.2 is not available on macOS by default", ) + @pytest.mark.parametrize("setting_value", [("127.0.0.2", 0), "127.0.0.2"]) @coroutine_test - async def test_download_bind_address_setting(self, mockserver: MockServer) -> None: + async def test_download_bind_address_setting( + self, mockserver: MockServer, setting_value: Any + ) -> None: request = Request(mockserver.url("/client-ip", is_secure=self.is_secure)) async with self.get_dh( - {"DOWNLOAD_BIND_ADDRESS": ("127.0.0.2", 0)} + {"DOWNLOAD_BIND_ADDRESS": setting_value} ) as download_handler: response = await download_handler.download_request(request) assert response.body == b"127.0.0.2" - # skip macOS tests @pytest.mark.skipif( sys.platform == "darwin", reason="127.0.0.2 is not available on macOS by default", ) + @pytest.mark.parametrize("meta_value", [("127.0.0.2", 0), "127.0.0.2"]) @coroutine_test - async def test_download_bind_address_setting_string( - self, mockserver: MockServer + async def test_download_bind_address_meta( + self, mockserver: MockServer, caplog: pytest.LogCaptureFixture, meta_value: Any ) -> None: - request = Request(mockserver.url("/client-ip", is_secure=self.is_secure)) - async with self.get_dh( - {"DOWNLOAD_BIND_ADDRESS": "127.0.0.2"} - ) as download_handler: + request = Request( + mockserver.url("/client-ip", is_secure=self.is_secure), + meta={"bindaddress": meta_value}, + ) + async with self.get_dh() as download_handler: response = await download_handler.download_request(request) - assert response.body == b"127.0.0.2" + if self.handler_supports_bindaddress_meta: + assert response.body == b"127.0.0.2" + else: + assert ( + "The 'bindaddress' request meta key is not supported by" in caplog.text + ) class TestHttps11Base(TestHttp11Base): diff --git a/tests/test_proxy_connect.py b/tests/test_proxy_connect.py index 912075401..c89b51ad3 100644 --- a/tests/test_proxy_connect.py +++ b/tests/test_proxy_connect.py @@ -20,7 +20,7 @@ class MitmProxy: auth_user = "scrapy" auth_pass = "scrapy" - def start(self): + def start(self) -> str: script = """ import sys from mitmproxy.tools.main import mitmdump @@ -28,34 +28,42 @@ sys.argv[0] = "mitmdump" sys.exit(mitmdump()) """ cert_path = Path(__file__).parent.resolve() / "keys" + args = [ + "--listen-host", + "127.0.0.1", + "--listen-port", + "0", + "--proxyauth", + f"{self.auth_user}:{self.auth_pass}", + "--set", + f"confdir={cert_path}", + "--ssl-insecure", + ] self.proc = Popen( [ sys.executable, "-u", "-c", script, - "--listen-host", - "127.0.0.1", - "--listen-port", - "0", - "--proxyauth", - f"{self.auth_user}:{self.auth_pass}", - "--set", - f"confdir={cert_path}", - "--ssl-insecure", + *args, ], stdout=PIPE, + text=True, ) - line = self.proc.stdout.readline().decode("utf-8") - host_port = re.search(r"listening at (?:http://)?([^:]+:\d+)", line).group(1) + assert self.proc.stdout is not None + line = self.proc.stdout.readline() + m = re.search(r"listening at (?:http://)?([^:]+:\d+)", line) + if not m: + raise RuntimeError(f"Failed to parse mitmdump output: {line}") + host_port = m.group(1) return f"http://{self.auth_user}:{self.auth_pass}@{host_port}" - def stop(self): + def stop(self) -> None: self.proc.kill() self.proc.communicate() -def _wrong_credentials(proxy_url): +def _wrong_credentials(proxy_url: str) -> str: bad_auth_proxy = list(urlsplit(proxy_url)) bad_auth_proxy[1] = bad_auth_proxy[1].replace("scrapy:scrapy@", "wrong:wronger@") return urlunsplit(bad_auth_proxy)