RomanNum3ral
/
scrapy
mirror of https://github.com/scrapy/scrapy.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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.
This commit is contained in:
Andrey Rakhmatullin 2026-04-21 18:27:16 +05:00 committed by GitHub
parent 9da14cdff1
commit 27092b2cb7
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
11 changed files with 144 additions and 75 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
scrapy/core/downloader/handlers/_httpx.py
View File

@ -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(

3
scrapy/core/http2/protocol.py
View File

@ -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

17
scrapy/core/http2/stream.py
View File

@ -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"],

5
tests/mockserver/dns.py
View File

@ -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):

7
tests/mockserver/http_base.py
View File

@ -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

2
tests/mockserver/http_resources.py
View File

@ -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")

16
tests/test_downloader_handler_httpx.py
View File

@ -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")

4
tests/test_downloader_handler_twisted_http10.py
View File

@ -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"

4
tests/test_downloader_handler_twisted_http2.py
View File

@ -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"

118
tests/test_downloader_handlers_http_base.py
View File

@ -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):

36
tests/test_proxy_connect.py
View File

@ -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)