Merge 6f13f9f475 into e28e56aa61

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 <topics-download-handlers>`, 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 <topics-download-handlers>`. 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

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

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)

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.downloader.handlers.base import BaseDownloadHandler
 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()

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

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

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 (
@@ -266,7 +270,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()
 
@@ -511,22 +517,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(

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}