From 0ccb609d6af7f3cc40de078eb0803c99bf05da41 Mon Sep 17 00:00:00 2001 From: Adrian Chaves Date: Wed, 12 Aug 2026 20:10:56 +0200 Subject: [PATCH] Handle data loss in HTTP/2 responses --- docs/topics/settings.rst | 12 ++---- scrapy/core/downloader/handlers/_httpx.py | 2 + scrapy/core/downloader/handlers/http2.py | 21 ++++++++-- scrapy/core/http2/stream.py | 25 ++++++++++-- tests/test_downloader_handler_httpx.py | 9 ----- .../test_downloader_handler_twisted_http2.py | 10 +---- tests/test_http2_client_protocol.py | 39 ++++++++++++------- tests/utils/bases/download_handlers_http.py | 15 ------- 8 files changed, 69 insertions(+), 64 deletions(-) diff --git a/docs/topics/settings.rst b/docs/topics/settings.rst index 010c9d1c6..e7ef7f51c 100644 --- a/docs/topics/settings.rst +++ b/docs/topics/settings.rst @@ -1185,6 +1185,9 @@ DOWNLOAD_FAIL_ON_DATALOSS Default: ``True`` +.. versionchanged:: VERSION + Added support for HTTP/2 responses. + Whether or not to fail on broken responses, that is, when the declared ``Content-Length`` does not match content sent by the server or a chunked response was not properly finished. If ``True``, these responses raise a @@ -1211,15 +1214,6 @@ Optionally, this can be set per-request basis by using the handler `, so it's not guaranteed to be supported by all 3rd-party handlers. -.. warning:: - - This setting is ignored by the - :class:`~scrapy.core.downloader.handlers.http2.H2DownloadHandler` - :ref:`download handler `. In case of a data loss - error, the corresponding HTTP/2 connection may be corrupted, affecting other - requests that use the same connection; hence, a ``ResponseFailed([InvalidBodyLengthError])`` - failure is always raised for every request that was using that connection. - .. setting:: DOWNLOAD_VERIFY_CERTIFICATES DOWNLOAD_VERIFY_CERTIFICATES diff --git a/scrapy/core/downloader/handlers/_httpx.py b/scrapy/core/downloader/handlers/_httpx.py index 8bbffb233..7d02e0bbd 100644 --- a/scrapy/core/downloader/handlers/_httpx.py +++ b/scrapy/core/downloader/handlers/_httpx.py @@ -215,6 +215,8 @@ class HttpxDownloadHandler(_Base): @staticmethod def _is_dataloss_exception(exc: Exception) -> bool: + if HAS_HTTP2 and isinstance(exc, h2.exceptions.InvalidBodyLengthError): + return True return isinstance( exc, httpx.RemoteProtocolError ) and "peer closed connection without sending complete message body" in str(exc) diff --git a/scrapy/core/downloader/handlers/http2.py b/scrapy/core/downloader/handlers/http2.py index 9b3d4fbd4..edf80781d 100644 --- a/scrapy/core/downloader/handlers/http2.py +++ b/scrapy/core/downloader/handlers/http2.py @@ -1,5 +1,6 @@ from __future__ import annotations +import logging from time import monotonic from typing import TYPE_CHECKING from urllib.parse import urldefrag @@ -10,9 +11,11 @@ from scrapy.core.http2.agent import H2Agent, H2ConnectionPool from scrapy.exceptions import ( DownloadTimeoutError, NotConfigured, + ResponseDataLossError, UnsupportedURLSchemeError, ) from scrapy.utils._download_handlers import ( + get_dataloss_msg, normalize_bind_address, wrap_twisted_exceptions, ) @@ -29,6 +32,9 @@ if TYPE_CHECKING: from scrapy.spiders import Spider +logger = logging.getLogger(__name__) + + class H2DownloadHandler(BaseDownloadHandler): lazy = True @@ -43,6 +49,7 @@ class H2DownloadHandler(BaseDownloadHandler): self._pool = H2ConnectionPool(reactor, crawler) self._context_factory = _load_context_factory_from_settings(crawler) self._bind_address = crawler.settings.get("DOWNLOAD_BIND_ADDRESS") + self._fail_on_dataloss_warned = False async def download_request(self, request: Request) -> Response: if urlparse_cached(request).scheme == "http": # pragma: no cover @@ -56,10 +63,16 @@ class H2DownloadHandler(BaseDownloadHandler): crawler=self._crawler, ) assert self._crawler.spider - with wrap_twisted_exceptions(): - return await maybe_deferred_to_future( - agent.download_request(request, self._crawler.spider) - ) + try: + with wrap_twisted_exceptions(): + return await maybe_deferred_to_future( + agent.download_request(request, self._crawler.spider) + ) + except ResponseDataLossError: + if not self._fail_on_dataloss_warned: + logger.warning(get_dataloss_msg(request.url)) + self._fail_on_dataloss_warned = True + raise async def close(self) -> None: self._pool.close_connections() diff --git a/scrapy/core/http2/stream.py b/scrapy/core/http2/stream.py index 4fc300d90..9e8e07a70 100644 --- a/scrapy/core/http2/stream.py +++ b/scrapy/core/http2/stream.py @@ -14,7 +14,11 @@ from twisted.python.failure import Failure from twisted.web.client import ResponseFailed from scrapy import signals -from scrapy.exceptions import DownloadCancelledError, StopDownload +from scrapy.exceptions import ( + DownloadCancelledError, + ResponseDataLossError, + StopDownload, +) from scrapy.http.headers import Headers from scrapy.utils._download_handlers import ( check_stop_download, @@ -129,6 +133,10 @@ class Stream: self._download_warnsize = self._request.meta.get( "download_warnsize", download_warnsize ) + self._fail_on_dataloss: bool = self._request.meta.get( + "download_fail_on_dataloss", + crawler.settings.getbool("DOWNLOAD_FAIL_ON_DATALOSS"), + ) # Metadata of an HTTP/2 connection stream # initialized when stream is instantiated @@ -511,7 +519,7 @@ class Stream: ) elif reason is StreamCloseReason.CONNECTION_LOST: - self._deferred_response.errback(ResponseFailed(errors)) + self._handle_connection_lost(errors) elif reason is StreamCloseReason.INACTIVE: errors = (InactiveStreamClosed(self._request), *errors) @@ -527,7 +535,17 @@ class Stream: ) ) - def _fire_response_deferred(self) -> None: + def _handle_connection_lost(self, errors: Sequence[BaseException]) -> None: + # A response body is only complete once the stream ends, so having + # received the response headers means that the body was cut short. + if self._response["status"] is None: + self._deferred_response.errback(ResponseFailed(errors)) + elif self._fail_on_dataloss: + self._deferred_response.errback(ResponseDataLossError(str(list(errors)))) + else: + self._fire_response_deferred(flags=["dataloss"]) + + def _fire_response_deferred(self, flags: list[str] | None = None) -> None: """Builds response from the self._response dict and fires the response deferred callback with the generated response instance""" @@ -538,6 +556,7 @@ class Stream: status=self._response["status"], headers=self._response["headers"], body=self._response["body"].getvalue(), + flags=flags, certificate=self._protocol.metadata["certificate"], ip_address=self._protocol.metadata["ip_address"], protocol="h2", diff --git a/tests/test_downloader_handler_httpx.py b/tests/test_downloader_handler_httpx.py index 27a44227d..77a3b3673 100644 --- a/tests/test_downloader_handler_httpx.py +++ b/tests/test_downloader_handler_httpx.py @@ -14,7 +14,6 @@ from scrapy.core.downloader.handlers._httpx import ( HAS_SOCKS, HttpxDownloadHandler, ) -from scrapy.exceptions import DownloadFailedError from scrapy.utils.misc import build_from_crawler from scrapy.utils.test import get_crawler from tests.utils.bases.download_handlers_http import ( @@ -94,7 +93,6 @@ class TestHttps(HttpxDownloadHandlerMixin, TestHttpsBase): @pytest.mark.skipif(not HAS_HTTP2, reason="No HTTP/2 support in HttpxDownloadHandler") class TestHttp2(TestHttps): http2 = True - handler_supports_http2_dataloss = False default_handler_settings: ClassVar[dict[str, Any]] = { "HTTPX_HTTP2_ENABLED": True, @@ -107,13 +105,6 @@ class TestHttp2(TestHttps): response = await download_handler.download_request(request) assert response.protocol == "HTTP/2" - @coroutine_test - async def test_data_loss_handling(self, mockserver: MockServer) -> None: - request = Request(mockserver.url("/broken", is_secure=self.is_secure)) - async with self.get_dh() as download_handler: - with pytest.raises(DownloadFailedError): - await download_handler.download_request(request) - class TestSimpleHttps(HttpxDownloadHandlerMixin, TestSimpleHttpsBase): pass diff --git a/tests/test_downloader_handler_twisted_http2.py b/tests/test_downloader_handler_twisted_http2.py index 2c3954b5e..284483d2d 100644 --- a/tests/test_downloader_handler_twisted_http2.py +++ b/tests/test_downloader_handler_twisted_http2.py @@ -11,7 +11,7 @@ from twisted.web.http import H2_ENABLED from scrapy import Spider from scrapy.crawler import Crawler -from scrapy.exceptions import DownloadFailedError, NotConfigured +from scrapy.exceptions import NotConfigured from scrapy.http import Request from scrapy.utils.misc import build_from_crawler from tests.utils.bases.download_handlers_http import ( @@ -72,7 +72,6 @@ def test_not_configured_without_reactor() -> None: class TestHttp2(H2DownloadHandlerMixin, TestHttpsBase): http2 = True - handler_supports_http2_dataloss = False @coroutine_test async def test_protocol(self, mockserver: MockServer) -> None: @@ -150,13 +149,6 @@ class TestHttp2(H2DownloadHandlerMixin, TestHttpsBase): f"{actual_content_length!r} instead", ) in caplog.record_tuples - @coroutine_test - async def test_data_loss_handling(self, mockserver: MockServer) -> None: - request = Request(mockserver.url("/broken", is_secure=self.is_secure)) - async with self.get_dh() as download_handler: - with pytest.raises(DownloadFailedError): - await download_handler.download_request(request) - class TestSimpleHttp2(H2DownloadHandlerMixin, TestSimpleHttpsBase): pass diff --git a/tests/test_http2_client_protocol.py b/tests/test_http2_client_protocol.py index 431b7a458..5f944d4d6 100644 --- a/tests/test_http2_client_protocol.py +++ b/tests/test_http2_client_protocol.py @@ -21,7 +21,11 @@ from twisted.web.http import Request as TxRequest from twisted.web.server import NOT_DONE_YET, Site from twisted.web.static import File -from scrapy.exceptions import DownloadCancelledError, DownloadTimeoutError +from scrapy.exceptions import ( + DownloadCancelledError, + DownloadTimeoutError, + ResponseDataLossError, +) from scrapy.http import JsonRequest, Request, Response from scrapy.spiders import Spider from scrapy.utils.defer import ( @@ -258,7 +262,9 @@ class TestHttps2ClientProtocol: yield client - if client.connected: + # H2ClientProtocol.connected stays set once the connection is made, so + # the underlying transport is what tells whether it is still open. + if client.transport.connected: client.transport.loseConnection() client.transport.abortConnection() @@ -492,22 +498,25 @@ class TestHttps2ClientProtocol: ): await make_request(client, request) - @inlineCallbacks - def test_received_dataloss_response( + @deferred_f_from_coro_f + async def test_received_dataloss_response( self, server_port: int, client: H2ClientProtocol - ) -> Generator[Deferred[Any], Any, None]: - """In case when value of Header Content-Length != len(Received Data) - ProtocolError is raised""" - from h2.exceptions import InvalidBodyLengthError # noqa: PLC0415 - + ) -> None: request = Request(url=self.get_url(server_port, "/dataloss")) - with pytest.raises(ResponseFailed) as exc_info: - yield make_request_dfd(client, request) - assert len(exc_info.value.reasons) > 0 - assert any( - isinstance(error, InvalidBodyLengthError) - for error in exc_info.value.reasons + with pytest.raises(ResponseDataLossError, match=r"InvalidBodyLengthError"): + await make_request(client, request) + + @deferred_f_from_coro_f + async def test_allow_dataloss_response( + self, server_port: int, client: H2ClientProtocol + ) -> None: + request = Request( + url=self.get_url(server_port, "/dataloss"), + meta={"download_fail_on_dataloss": False}, ) + response = await make_request(client, request) + assert response.flags == ["dataloss"] + assert response.body == Data.DATALOSS @deferred_f_from_coro_f async def test_missing_content_length_header( diff --git a/tests/utils/bases/download_handlers_http.py b/tests/utils/bases/download_handlers_http.py index 9b4a38724..28bcee5d6 100644 --- a/tests/utils/bases/download_handlers_http.py +++ b/tests/utils/bases/download_handlers_http.py @@ -68,13 +68,6 @@ class TestHttpBase(ABC): http2: bool = False # whether the handler supports per-request bindaddress handler_supports_bindaddress_meta: bool = True - # RFC 9113 ยง8.1.1 explicitly says that a Content-Length mismatch is a - # stream error (of type PROTOCOL_ERROR) so the client will send - # RST_STREAM. Some libraries do only this while e.g. h2 also closes the - # connection (see handling of ProtocolError in - # h2.connection.H2Connection.receive_data()), thus closing all streams that - # were using it, and we handle this as a normal exception. - handler_supports_http2_dataloss: bool = True # What the handler does with a bad response header line, e.g. one with no # colon in it: # "skip-bad": the bad line is skipped and the header lines that follow it @@ -691,8 +684,6 @@ class TestHttpBase(ABC): @coroutine_test async def test_download_cause_data_loss(self, mockserver: MockServer) -> None: - if self.http2 and not self.handler_supports_http2_dataloss: - pytest.skip("This handler doesn't support dataloss on HTTP/2") request = Request(mockserver.url("/broken", is_secure=self.is_secure)) async with self.get_dh() as download_handler: with pytest.raises(ResponseDataLossError): @@ -702,8 +693,6 @@ class TestHttpBase(ABC): async def test_download_cause_data_loss_double_warning( self, caplog: pytest.LogCaptureFixture, mockserver: MockServer ) -> None: - if self.http2 and not self.handler_supports_http2_dataloss: - pytest.skip("This handler doesn't support dataloss on HTTP/2") request = Request(mockserver.url("/broken", is_secure=self.is_secure)) async with self.get_dh() as download_handler: with pytest.raises(ResponseDataLossError): @@ -719,8 +708,6 @@ class TestHttpBase(ABC): async def test_download_allow_data_loss_broken( self, mockserver: MockServer ) -> None: - if self.http2 and not self.handler_supports_http2_dataloss: - pytest.skip("This handler doesn't support dataloss on HTTP/2") request = Request( mockserver.url("/broken", is_secure=self.is_secure), meta={"download_fail_on_dataloss": False}, @@ -749,8 +736,6 @@ class TestHttpBase(ABC): async def test_download_allow_data_loss_via_setting( self, mockserver: MockServer ) -> None: - if self.http2 and not self.handler_supports_http2_dataloss: - pytest.skip("This handler doesn't support dataloss on HTTP/2") request = Request(mockserver.url("/broken", is_secure=self.is_secure)) async with self.get_dh( {"DOWNLOAD_FAIL_ON_DATALOSS": False}