Merge remote-tracking branch 'origin/master' into throttling

scrapy/selector/unified.py

@@ -1,10 +1,6 @@
-"""
-XPath selectors based on lxml
-"""
-
 from __future__ import annotations
 
-from typing import Any
+from typing import Any, Literal
 
 from parsel import Selector as _ParselSelector
 
@@ -18,13 +14,10 @@ __all__ = ["Selector", "SelectorList"]
 _NOT_SET = object()
 
 
-def _st(response: TextResponse | None, st: str | None) -> str:
-    if st is None:
-        return "xml" if isinstance(response, XmlResponse) else "html"
-    return st
+SelectorType = Literal["html", "xml", "json", "text"]
 
 
-def _response_from_text(text: str | bytes, st: str | None) -> TextResponse:
+def _response_from_text(text: str | bytes, st: SelectorType | None) -> TextResponse:
     rt: type[TextResponse] = XmlResponse if st == "xml" else HtmlResponse
     return rt(url="about:blank", encoding="utf-8", body=to_bytes(text, "utf-8"))
 
@@ -49,23 +42,16 @@ class Selector(_ParselSelector, object_ref):
     ``response`` isn't available. Using ``text`` and ``response`` together is
     undefined behavior.
 
-    ``type`` defines the selector type, it can be ``"html"``, ``"xml"``, ``"json"``
-    or ``None`` (default).
+    ``type`` defines the selector type, it can be ``"html"``, ``"xml"``,
+    ``"json"``, ``"text"`` or ``None`` (default). It's passed to
+    :class:`parsel.Selector` and its meaning is defined there. However, when
+    ``type`` is ``None``, it is set to ``"xml"`` for an
+    :class:`~scrapy.http.XmlResponse` and to ``"html"`` otherwise before
+    passing it to :class:`parsel.Selector`.
 
-    If ``type`` is ``None``, the selector automatically chooses the best type
-    based on ``response`` type (see below), or defaults to ``"html"`` in case it
-    is used together with ``text``.
-
-    If ``type`` is ``None`` and a ``response`` is passed, the selector type is
-    inferred from the response type as follows:
-
-    * ``"html"`` for :class:`~scrapy.http.HtmlResponse` type
-    * ``"xml"`` for :class:`~scrapy.http.XmlResponse` type
-    * ``"json"`` for :class:`~scrapy.http.TextResponse` type
-    * ``"html"`` for anything else
-
-    Otherwise, if ``type`` is set, the selector type will be forced and no
-    detection will occur.
+    .. note:: JSON selector support requires ``parsel`` 1.8.0 or higher. With
+       older versions setting ``type`` to ``"json"`` or ``"text"`` is not
+       supported.
     """
 
     __slots__ = ["response"]
@@ -75,7 +61,7 @@ class Selector(_ParselSelector, object_ref):
         self,
         response: TextResponse | None = None,
         text: str | None = None,
-        type: str | None = None,  # noqa: A002
+        type: SelectorType | None = None,  # noqa: A002
         root: Any | None = _NOT_SET,
         **kwargs: Any,
     ):
@@ -84,10 +70,11 @@ class Selector(_ParselSelector, object_ref):
                 f"{self.__class__.__name__}.__init__() received both response and text"
             )
 
-        st = _st(response, type)
+        if type is None:
+            type = "xml" if isinstance(response, XmlResponse) else "html"  # noqa: A001
 
         if text is not None:
-            response = _response_from_text(text, st)
+            response = _response_from_text(text, type)
 
         if response is not None:
             text = response.text
@@ -98,4 +85,4 @@ class Selector(_ParselSelector, object_ref):
         if root is not _NOT_SET:
             kwargs["root"] = root
 
-        super().__init__(text=text, type=st, **kwargs)
+        super().__init__(text=text, type=type, **kwargs)

tests/AsyncCrawlerProcess/asyncio_enabled_reactor_same_loop_default.py

@@ -0,0 +1,31 @@
+import asyncio
+import sys
+
+from twisted.internet import asyncioreactor
+
+import scrapy
+from scrapy.crawler import AsyncCrawlerProcess
+
+if sys.platform == "win32":
+    asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy())
+loop = asyncio.SelectorEventLoop()
+asyncio.set_event_loop(loop)
+asyncioreactor.install(loop)
+
+
+class NoRequestsSpider(scrapy.Spider):
+    name = "no_request"
+
+    async def start(self):
+        return
+        yield
+
+
+process = AsyncCrawlerProcess(
+    settings={
+        "TWISTED_REACTOR": "twisted.internet.asyncioreactor.AsyncioSelectorReactor",
+        "ASYNCIO_EVENT_LOOP": "asyncio.SelectorEventLoop",
+    }
+)
+process.crawl(NoRequestsSpider)
+process.start()

tests/AsyncCrawlerProcess/dns_resolver_deprecated.py

@@ -0,0 +1,31 @@
+import sys
+
+import scrapy
+from scrapy.crawler import AsyncCrawlerProcess
+from scrapy.settings import Settings
+
+
+class NoRequestsSpider(scrapy.Spider):
+    name = "no_request"
+
+    async def start(self):
+        return
+        yield
+
+
+settings = Settings()
+# The deprecated DNS_RESOLVER setting, set above its default priority so that
+# AsyncCrawlerProcess._setup_reactor() emits the deprecation warning.
+settings.set("DNS_RESOLVER", "scrapy.resolver.CachingThreadedResolver", priority=10)
+if len(sys.argv) > 1 and sys.argv[1] == "twisted-wins":
+    # TWISTED_DNS_RESOLVER at a higher priority takes precedence over the
+    # deprecated DNS_RESOLVER setting.
+    settings.set(
+        "TWISTED_DNS_RESOLVER",
+        "scrapy.resolver.CachingThreadedResolver",
+        priority=20,
+    )
+
+process = AsyncCrawlerProcess(settings)
+process.crawl(NoRequestsSpider)
+process.start()

tests/AsyncCrawlerProcess/reactorless_sleeping.py

@@ -17,4 +17,4 @@ class SleepingSpider(scrapy.Spider):
 process = AsyncCrawlerProcess(settings={"TWISTED_REACTOR_ENABLED": False})
 
 process.crawl(SleepingSpider)
-process.start()
+process.start(stop_after_crawl="--no-stop" not in sys.argv)

tests/AsyncCrawlerProcess/sleeping.py

@@ -17,4 +17,4 @@ class SleepingSpider(scrapy.Spider):
 process = AsyncCrawlerProcess(settings={})
 
 process.crawl(SleepingSpider)
-process.start()
+process.start(stop_after_crawl="--no-stop" not in sys.argv)

tests/CrawlerProcess/dns_resolver_deprecated.py

@@ -0,0 +1,31 @@
+import sys
+
+import scrapy
+from scrapy.crawler import CrawlerProcess
+from scrapy.settings import Settings
+
+
+class NoRequestsSpider(scrapy.Spider):
+    name = "no_request"
+
+    async def start(self):
+        return
+        yield
+
+
+settings = Settings()
+# The deprecated DNS_RESOLVER setting, set above its default priority so that
+# CrawlerProcess._setup_reactor() emits the deprecation warning.
+settings.set("DNS_RESOLVER", "scrapy.resolver.CachingThreadedResolver", priority=10)
+if len(sys.argv) > 1 and sys.argv[1] == "twisted-wins":
+    # TWISTED_DNS_RESOLVER at a higher priority takes precedence over the
+    # deprecated DNS_RESOLVER setting.
+    settings.set(
+        "TWISTED_DNS_RESOLVER",
+        "scrapy.resolver.CachingThreadedResolver",
+        priority=20,
+    )
+
+process = CrawlerProcess(settings)
+process.crawl(NoRequestsSpider)
+process.start()

tests/CrawlerProcess/sleeping.py

@@ -23,4 +23,4 @@ class SleepingSpider(scrapy.Spider):
 process = CrawlerProcess(settings={})
 
 process.crawl(SleepingSpider)
-process.start()
+process.start(stop_after_crawl="--no-stop" not in sys.argv)

tests/test_crawler.py

@@ -3,8 +3,11 @@ from __future__ import annotations
 import asyncio
 import logging
 import re
+import signal
+import threading
 from pathlib import Path
-from typing import Any, ClassVar
+from typing import TYPE_CHECKING, Any, ClassVar
+from unittest.mock import MagicMock
 
 import pytest
 from zope.interface.exceptions import MultipleInvalid
@@ -32,6 +35,9 @@ from scrapy.utils.spider import DefaultSpider
 from scrapy.utils.test import get_crawler, get_reactor_settings
 from tests.utils.decorators import coroutine_test
 
+if TYPE_CHECKING:
+    from collections.abc import Callable
+
 BASE_SETTINGS: dict[str, Any] = {}
 
 
@@ -675,6 +681,145 @@ class TestAsyncCrawlerProcess(TestBaseCrawler):
         self.assertOptionIsDefault(runner.settings, "RETRY_ENABLED")
 
 
+class TestAsyncCrawlerProcessReactorlessHelpers:
+    """Unit tests for the reactorless shutdown helpers of AsyncCrawlerProcess.
+
+    These cover defensive branches that guard against shutdown races and that
+    are not reachable through a full process run.
+    """
+
+    @staticmethod
+    def _bare_process(
+        monkeypatch: pytest.MonkeyPatch,
+    ) -> tuple[AsyncCrawlerProcess, list[Any]]:
+        # AsyncCrawlerProcess.__init__ has global side effects (it installs a
+        # reactor import hook and an asyncio event loop), so build a bare
+        # instance and set only the attributes these helpers read. The shutdown
+        # handlers installed by these helpers are recorded for assertions
+        # instead of touching the real process-wide signal handlers.
+        installed_handlers: list[Any] = []
+        monkeypatch.setattr(
+            "scrapy.crawler.install_shutdown_handlers",
+            lambda handler, *args, **kwargs: installed_handlers.append(handler),
+        )
+        return AsyncCrawlerProcess.__new__(AsyncCrawlerProcess), installed_handlers
+
+    @staticmethod
+    def _run_in_thread(target: Callable[[], None]) -> None:
+        # Run target in a dedicated thread so its event loop is not nested
+        # inside the event loop that may already be running the test session.
+        thread = threading.Thread(target=target)
+        thread.start()
+        thread.join()
+
+    def test_signal_shutdown_reactorless_without_loop(
+        self, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        process, installed_handlers = self._bare_process(monkeypatch)
+        process._reactorless_loop = None
+        # No loop to schedule the shutdown task on, so it returns early, but it
+        # must still escalate the handler so a second signal forces a kill.
+        process._signal_shutdown_reactorless(signal.SIGINT, None)
+        assert installed_handlers == [process._signal_kill_reactorless]
+
+    def test_signal_kill_reactorless_without_loop(
+        self, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        process, installed_handlers = self._bare_process(monkeypatch)
+        process._reactorless_loop = None
+        process._reactorless_main_task = None
+        # No loop to cancel the main task on, so it returns early, but it must
+        # still ignore any further signals.
+        process._signal_kill_reactorless(signal.SIGINT, None)
+        assert installed_handlers == [signal.SIG_IGN]
+
+    def test_signal_kill_reactorless_without_main_task(
+        self, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        process, installed_handlers = self._bare_process(monkeypatch)
+        loop = MagicMock()
+        process._reactorless_loop = loop
+        process._reactorless_main_task = None
+        # No main task to cancel, so nothing is scheduled on the loop.
+        process._signal_kill_reactorless(signal.SIGINT, None)
+        assert installed_handlers == [signal.SIG_IGN]
+        loop.call_soon_threadsafe.assert_not_called()
+
+    def test_shutdown_graceful_reactorless_main_task_already_done(
+        self, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        process, _ = self._bare_process(monkeypatch)
+        process._stop_after_crawl = False
+
+        async def noop() -> None:
+            return None
+
+        monkeypatch.setattr(process, "stop", noop)
+        monkeypatch.setattr(process, "join", noop)
+
+        def run() -> None:
+            loop = asyncio.new_event_loop()
+            try:
+                main_task: asyncio.Future[None] = loop.create_future()
+                main_task.set_result(None)
+                process._reactorless_main_task = main_task
+                # The main task is already done, so it is not cancelled.
+                loop.run_until_complete(process._shutdown_graceful_reactorless())
+                assert not main_task.cancelled()
+            finally:
+                loop.close()
+
+        self._run_in_thread(run)
+
+    def test_create_shutdown_task_closed_loop(
+        self, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        process, _ = self._bare_process(monkeypatch)
+        loop = asyncio.new_event_loop()
+        loop.close()
+        process._reactorless_loop = loop
+        process._stop_after_crawl = True
+        # create_task() raises RuntimeError on a closed loop; the coroutine
+        # must be closed instead of leaking.
+        process._create_shutdown_task()
+
+    def test_cancel_all_tasks_logs_task_exception(self) -> None:
+        contexts: list[dict[str, Any]] = []
+        task_was_cancelled: list[bool] = []
+
+        def run() -> None:
+            loop = asyncio.new_event_loop()
+            loop.set_exception_handler(lambda _loop, context: contexts.append(context))
+
+            async def fail_on_cancel() -> None:
+                try:
+                    await asyncio.sleep(10)
+                except asyncio.CancelledError:
+                    raise RuntimeError("boom")
+
+            try:
+                task = loop.create_task(fail_on_cancel())
+                # Let the task start and suspend on the sleep so the
+                # cancellation is raised inside its body and turned into a
+                # RuntimeError rather than cancelling the task cleanly.
+                loop.run_until_complete(asyncio.sleep(0))
+                AsyncCrawlerProcess._cancel_all_tasks(loop)
+                task_was_cancelled.append(task.cancelled())
+            finally:
+                loop.close()
+
+        self._run_in_thread(run)
+
+        # The task raised instead of being cancelled, so its exception is
+        # reported to the loop exception handler.
+        assert task_was_cancelled == [False]
+        assert any(
+            context.get("message")
+            == "unhandled exception during AsyncCrawlerProcess shutdown"
+            for context in contexts
+        )
+
+
 @pytest.mark.parametrize("runner_cls", [AsyncCrawlerRunner, CrawlerRunner])
 def test_runner_settings_applied_to_crawler_instance(
     runner_cls: type[CrawlerRunnerBase],
@@ -711,6 +856,22 @@ def test_create_crawler_instance_consistent_with_spider_class() -> None:
     assert pre_built.settings["FOO"] == "runner"
 
 
+@pytest.mark.parametrize("runner_cls", [AsyncCrawlerRunner, CrawlerRunner])
+def test_create_crawler_rejects_spider_object(
+    runner_cls: type[CrawlerRunnerBase],
+) -> None:
+    runner = runner_cls()
+    with pytest.raises(ValueError, match="cannot be a spider object"):
+        runner.create_crawler(DefaultSpider())  # type: ignore[arg-type]
+
+
+@pytest.mark.parametrize("runner_cls", [AsyncCrawlerRunner, CrawlerRunner])
+def test_crawl_rejects_spider_object(runner_cls: type[CrawlerRunnerBase]) -> None:
+    runner = runner_cls()
+    with pytest.raises(ValueError, match="cannot be a spider object"):
+        runner.crawl(DefaultSpider())  # type: ignore[arg-type]
+
+
 class ExceptionSpider(scrapy.Spider):
     name = "exception"
 

tests/test_crawler_subprocess.py

@@ -126,6 +126,16 @@ class TestCrawlerProcessSubprocessBase(ScriptRunnerMixin):
         assert "TimeoutError" not in log
         assert "scrapy.exceptions.CannotResolveHostError" not in log
 
+    def test_dns_resolver_deprecated(self) -> None:
+        log = self.run_script("dns_resolver_deprecated.py")
+        assert "Spider closed (finished)" in log
+        assert "The DNS_RESOLVER setting is deprecated" in log
+
+    def test_dns_resolver_deprecated_twisted_dns_resolver(self) -> None:
+        log = self.run_script("dns_resolver_deprecated.py", "twisted-wins")
+        assert "Spider closed (finished)" in log
+        assert "The DNS_RESOLVER setting is deprecated" in log
+
     def test_twisted_reactor_asyncio(self) -> None:
         log = self.run_script("twisted_reactor_asyncio.py")
         assert "Spider closed (finished)" in log
@@ -205,9 +215,11 @@ class TestCrawlerProcessSubprocessBase(ScriptRunnerMixin):
         assert "Spider closed (finished)" in log
         assert "The value of FOO is 42" in log
 
-    def _test_shutdown_graceful(self, script: str = "sleeping.py") -> None:
+    def _test_shutdown_graceful(
+        self, script: str = "sleeping.py", *extra_args: str
+    ) -> None:
         sig = signal.SIGINT if sys.platform != "win32" else signal.SIGBREAK  # type: ignore[attr-defined]
-        args = self.get_script_args(script, "3")
+        args = self.get_script_args(script, "3", *extra_args)
         p = PopenSpawn(args, timeout=5, env=get_script_run_env())
         p.expect_exact("Spider opened")
         p.expect_exact("Crawled (200)")
@@ -245,6 +257,9 @@ class TestCrawlerProcessSubprocessBase(ScriptRunnerMixin):
     async def test_shutdown_forced(self) -> None:
         await self._test_shutdown_forced()
 
+    def test_shutdown_graceful_no_stop(self) -> None:
+        self._test_shutdown_graceful("sleeping.py", "--no-stop")
+
 
 class TestCrawlerProcessSubprocess(TestCrawlerProcessSubprocessBase):
     @property
@@ -429,6 +444,17 @@ class TestAsyncCrawlerProcessSubprocess(TestCrawlerProcessSubprocessBase):
     async def test_shutdown_forced(self) -> None:
         await self._test_shutdown_forced("reactorless_sleeping.py")
 
+    def test_shutdown_graceful_reactorless_no_stop(self) -> None:
+        self._test_shutdown_graceful("reactorless_sleeping.py", "--no-stop")
+
+    def test_asyncio_enabled_reactor_same_loop_default(self) -> None:
+        log = self.run_script("asyncio_enabled_reactor_same_loop_default.py")
+        assert "Spider closed (finished)" in log
+        assert (
+            "Using reactor: twisted.internet.asyncioreactor.AsyncioSelectorReactor"
+            in log
+        )
+
 
 class TestCrawlerRunnerSubprocessBase(ScriptRunnerMixin):
     """Common tests between CrawlerRunner and AsyncCrawlerRunner,