From 720f351a3eea5e5bfa83a6eaf50210cd1fa43992 Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Sun, 24 Sep 2023 18:12:52 +0400 Subject: [PATCH 1/9] Refactor installing signals. --- scrapy/crawler.py | 8 +++++--- scrapy/utils/ossignal.py | 9 +++------ 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/scrapy/crawler.py b/scrapy/crawler.py index 22fd65be7..15aec2757 100644 --- a/scrapy/crawler.py +++ b/scrapy/crawler.py @@ -416,15 +416,17 @@ class CrawlerProcess(CrawlerRunner): return d.addBoth(self._stop_reactor) - if install_signal_handlers: - install_shutdown_handlers(self._signal_shutdown) resolver_class = load_object(self.settings["DNS_RESOLVER"]) resolver = create_instance(resolver_class, self.settings, self, reactor=reactor) resolver.install_on_reactor() tp = reactor.getThreadPool() tp.adjustPoolsize(maxthreads=self.settings.getint("REACTOR_THREADPOOL_MAXSIZE")) reactor.addSystemEventTrigger("before", "shutdown", self.stop) - reactor.run(installSignalHandlers=False) # blocking call + if install_signal_handlers: + reactor.addSystemEventTrigger( + "after", "startup", install_shutdown_handlers, self._signal_shutdown + ) + reactor.run() # blocking call def _graceful_stop_reactor(self) -> Deferred: d = self.stop() diff --git a/scrapy/utils/ossignal.py b/scrapy/utils/ossignal.py index 2334ea792..db9a71273 100644 --- a/scrapy/utils/ossignal.py +++ b/scrapy/utils/ossignal.py @@ -19,13 +19,10 @@ def install_shutdown_handlers( function: SignalHandlerT, override_sigint: bool = True ) -> None: """Install the given function as a signal handler for all common shutdown - signals (such as SIGINT, SIGTERM, etc). If override_sigint is ``False`` the - SIGINT handler won't be install if there is already a handler in place - (e.g. Pdb) + signals (such as SIGINT, SIGTERM, etc). If ``override_sigint`` is ``False`` the + SIGINT handler won't be installed if there is already a handler in place + (e.g. Pdb) """ - from twisted.internet import reactor - - reactor._handleSignals() signal.signal(signal.SIGTERM, function) if signal.getsignal(signal.SIGINT) == signal.default_int_handler or override_sigint: signal.signal(signal.SIGINT, function) From 4b14215f83996a18d34c95bec953ef9cfba05245 Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Sun, 24 Sep 2023 18:17:43 +0400 Subject: [PATCH 2/9] Remove the Twisted version restriction. --- setup.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/setup.py b/setup.py index 47c0af0b0..405633f55 100644 --- a/setup.py +++ b/setup.py @@ -6,8 +6,7 @@ version = (Path(__file__).parent / "scrapy/VERSION").read_text("ascii").strip() install_requires = [ - # 23.8.0 incompatibility: https://github.com/scrapy/scrapy/issues/6024 - "Twisted>=18.9.0,<23.8.0", + "Twisted>=18.9.0", "cryptography>=36.0.0", "cssselect>=0.9.1", "itemloaders>=1.0.1", From 0c6440a427a6f3ed2920de38f97d15d692833851 Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Sun, 24 Sep 2023 19:25:59 +0400 Subject: [PATCH 3/9] Fix additional typing errors with new Twisted. --- scrapy/utils/testproc.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/scrapy/utils/testproc.py b/scrapy/utils/testproc.py index 5f7a7db14..0688e014b 100644 --- a/scrapy/utils/testproc.py +++ b/scrapy/utils/testproc.py @@ -2,7 +2,7 @@ from __future__ import annotations import os import sys -from typing import Iterable, Optional, Tuple, cast +from typing import Iterable, List, Optional, Tuple, cast from twisted.internet.defer import Deferred from twisted.internet.error import ProcessTerminated @@ -26,14 +26,15 @@ class ProcessTest: env = os.environ.copy() if settings is not None: env["SCRAPY_SETTINGS_MODULE"] = settings + assert self.command cmd = self.prefix + [self.command] + list(args) pp = TestProcessProtocol() - pp.deferred.addBoth(self._process_finished, cmd, check_code) + pp.deferred.addCallback(self._process_finished, cmd, check_code) reactor.spawnProcess(pp, cmd[0], cmd, env=env, path=self.cwd) return pp.deferred def _process_finished( - self, pp: TestProcessProtocol, cmd: str, check_code: bool + self, pp: TestProcessProtocol, cmd: List[str], check_code: bool ) -> Tuple[int, bytes, bytes]: if pp.exitcode and check_code: msg = f"process {cmd} exit with code {pp.exitcode}" From 0630e4aaa10c3fb8c79c2542a229f5c0632cddde Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Sun, 24 Sep 2023 19:36:29 +0400 Subject: [PATCH 4/9] Fix `scrapy shell`. --- scrapy/crawler.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scrapy/crawler.py b/scrapy/crawler.py index 15aec2757..6f54e62e9 100644 --- a/scrapy/crawler.py +++ b/scrapy/crawler.py @@ -404,8 +404,8 @@ class CrawlerProcess(CrawlerRunner): :param bool stop_after_crawl: stop or not the reactor when all crawlers have finished - :param bool install_signal_handlers: whether to install the shutdown - handlers (default: True) + :param bool install_signal_handlers: whether to install the OS signal + handlers from Twisted and Scrapy (default: True) """ from twisted.internet import reactor @@ -426,7 +426,7 @@ class CrawlerProcess(CrawlerRunner): reactor.addSystemEventTrigger( "after", "startup", install_shutdown_handlers, self._signal_shutdown ) - reactor.run() # blocking call + reactor.run(installSignalHandlers=install_signal_handlers) # blocking call def _graceful_stop_reactor(self) -> Deferred: d = self.stop() From d19e315b0b07b070f45cf3c082d4dfe0e6a87186 Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Sun, 24 Sep 2023 21:51:19 +0400 Subject: [PATCH 5/9] Add an interactive test for `scrapy shell`. --- tests/requirements.txt | 1 + tests/test_command_shell.py | 26 ++++++++++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/tests/requirements.txt b/tests/requirements.txt index 3ea7f3333..dc004f3f1 100644 --- a/tests/requirements.txt +++ b/tests/requirements.txt @@ -1,5 +1,6 @@ # Tests requirements attrs +pexpect >= 4.8.0 # https://github.com/giampaolo/pyftpdlib/issues/560 pyftpdlib; python_version < "3.12" pytest diff --git a/tests/test_command_shell.py b/tests/test_command_shell.py index 6589381f3..7d87eb62c 100644 --- a/tests/test_command_shell.py +++ b/tests/test_command_shell.py @@ -1,11 +1,15 @@ +import sys +from io import BytesIO from pathlib import Path +from pexpect.popen_spawn import PopenSpawn from twisted.internet import defer from twisted.trial import unittest from scrapy.utils.testproc import ProcessTest from scrapy.utils.testsite import SiteTest from tests import NON_EXISTING_RESOLVABLE, tests_datadir +from tests.mockserver import MockServer class ShellTest(ProcessTest, SiteTest, unittest.TestCase): @@ -133,3 +137,25 @@ class ShellTest(ProcessTest, SiteTest, unittest.TestCase): args = ["-c", code, "--set", f"TWISTED_REACTOR={reactor_path}"] _, _, err = yield self.execute(args, check_code=True) self.assertNotIn(b"RuntimeError: There is no current event loop in thread", err) + + +class InteractiveShellTest(unittest.TestCase): + def test_fetch(self): + args = ( + sys.executable, + "-m", + "scrapy.cmdline", + "shell", + ) + logfile = BytesIO() + p = PopenSpawn(args, timeout=5) + p.logfile_read = logfile + p.expect_exact("Available Scrapy objects") + with MockServer() as mockserver: + p.sendline(f"fetch('{mockserver.url('/')}')") + p.sendline("type(response)") + p.expect_exact("HtmlResponse") + p.sendeof() + p.wait() + logfile.seek(0) + self.assertNotIn("Traceback", logfile.read().decode()) From 4b5fb9b5a6c7738aee22b5080fd96588feea39a5 Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Sun, 24 Sep 2023 22:52:17 +0400 Subject: [PATCH 6/9] Add a test for SIGTERM handling. --- tests/CrawlerProcess/sleeping.py | 24 ++++++++++++++++++++++++ tests/test_crawler.py | 31 +++++++++++++++++++++++++++++-- 2 files changed, 53 insertions(+), 2 deletions(-) create mode 100644 tests/CrawlerProcess/sleeping.py diff --git a/tests/CrawlerProcess/sleeping.py b/tests/CrawlerProcess/sleeping.py new file mode 100644 index 000000000..b46f7ee2d --- /dev/null +++ b/tests/CrawlerProcess/sleeping.py @@ -0,0 +1,24 @@ +from twisted.internet.defer import Deferred + +import scrapy +from scrapy.crawler import CrawlerProcess +from scrapy.utils.defer import maybe_deferred_to_future + + +class SleepingSpider(scrapy.Spider): + name = "sleeping" + + start_urls = ["data:,;"] + + async def parse(self, response): + from twisted.internet import reactor + + d = Deferred() + reactor.callLater(2, d.callback, None) + await maybe_deferred_to_future(d) + + +process = CrawlerProcess(settings={}) + +process.crawl(SleepingSpider) +process.start() diff --git a/tests/test_crawler.py b/tests/test_crawler.py index 2b141e894..f0a308d95 100644 --- a/tests/test_crawler.py +++ b/tests/test_crawler.py @@ -1,13 +1,16 @@ import logging import os import platform +import signal import subprocess import sys import warnings from pathlib import Path +from typing import List import pytest from packaging.version import parse as parse_version +from pexpect.popen_spawn import PopenSpawn from pytest import mark, raises from twisted.internet import defer from twisted.trial import unittest @@ -289,9 +292,12 @@ class ScriptRunnerMixin: script_dir: Path cwd = os.getcwd() - def run_script(self, script_name: str, *script_args): + def get_script_args(self, script_name: str, *script_args: str) -> List[str]: script_path = self.script_dir / script_name - args = [sys.executable, str(script_path)] + list(script_args) + return [sys.executable, str(script_path)] + list(script_args) + + def run_script(self, script_name: str, *script_args: str) -> str: + args = self.get_script_args(script_name, *script_args) p = subprocess.Popen( args, env=get_mockserver_env(), @@ -517,6 +523,27 @@ class CrawlerProcessSubprocess(ScriptRunnerMixin, unittest.TestCase): self.assertIn("Spider closed (finished)", log) self.assertIn("The value of FOO is 42", log) + def test_shutdown_graceful(self): + args = self.get_script_args("sleeping.py") + p = PopenSpawn(args, timeout=5) + p.expect_exact("Spider opened") + p.expect_exact("Crawled (200)") + p.kill(signal.SIGTERM) + p.expect_exact("shutting down gracefully") + p.expect_exact("Spider closed (shutdown)") + p.wait() + + def test_shutdown_forced(self): + args = self.get_script_args("sleeping.py") + p = PopenSpawn(args, timeout=5) + p.expect_exact("Spider opened") + p.expect_exact("Crawled (200)") + p.kill(signal.SIGTERM) + p.expect_exact("shutting down gracefully") + p.kill(signal.SIGTERM) + p.expect_exact("forcing unclean shutdown") + p.wait() + class CrawlerRunnerSubprocess(ScriptRunnerMixin, unittest.TestCase): script_dir = Path(__file__).parent.resolve() / "CrawlerRunner" From eb5e2e79ba8d2c492737a6e2ab8e0d4de2bd1a6e Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Sun, 24 Sep 2023 23:31:46 +0400 Subject: [PATCH 7/9] Use SIGINT instead of SIGTERM to support Windows. --- tests/test_crawler.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_crawler.py b/tests/test_crawler.py index f0a308d95..b43a5826c 100644 --- a/tests/test_crawler.py +++ b/tests/test_crawler.py @@ -528,7 +528,7 @@ class CrawlerProcessSubprocess(ScriptRunnerMixin, unittest.TestCase): p = PopenSpawn(args, timeout=5) p.expect_exact("Spider opened") p.expect_exact("Crawled (200)") - p.kill(signal.SIGTERM) + p.kill(signal.SIGINT) p.expect_exact("shutting down gracefully") p.expect_exact("Spider closed (shutdown)") p.wait() @@ -538,9 +538,9 @@ class CrawlerProcessSubprocess(ScriptRunnerMixin, unittest.TestCase): p = PopenSpawn(args, timeout=5) p.expect_exact("Spider opened") p.expect_exact("Crawled (200)") - p.kill(signal.SIGTERM) + p.kill(signal.SIGINT) p.expect_exact("shutting down gracefully") - p.kill(signal.SIGTERM) + p.kill(signal.SIGINT) p.expect_exact("forcing unclean shutdown") p.wait() From 2f436c05e088b063b70f000a638409772df27138 Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Sun, 8 Oct 2023 22:55:05 +0400 Subject: [PATCH 8/9] Use SIGBREAK in Windows tests. --- tests/test_crawler.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/test_crawler.py b/tests/test_crawler.py index b43a5826c..60b92377d 100644 --- a/tests/test_crawler.py +++ b/tests/test_crawler.py @@ -524,23 +524,25 @@ class CrawlerProcessSubprocess(ScriptRunnerMixin, unittest.TestCase): self.assertIn("The value of FOO is 42", log) def test_shutdown_graceful(self): + sig = signal.SIGINT if sys.platform != "win32" else signal.SIGBREAK args = self.get_script_args("sleeping.py") p = PopenSpawn(args, timeout=5) p.expect_exact("Spider opened") p.expect_exact("Crawled (200)") - p.kill(signal.SIGINT) + p.kill(sig) p.expect_exact("shutting down gracefully") p.expect_exact("Spider closed (shutdown)") p.wait() def test_shutdown_forced(self): + sig = signal.SIGINT if sys.platform != "win32" else signal.SIGBREAK args = self.get_script_args("sleeping.py") p = PopenSpawn(args, timeout=5) p.expect_exact("Spider opened") p.expect_exact("Crawled (200)") - p.kill(signal.SIGINT) + p.kill(sig) p.expect_exact("shutting down gracefully") - p.kill(signal.SIGINT) + p.kill(sig) p.expect_exact("forcing unclean shutdown") p.wait() From 029a56384dcac0a52fe40f1bebaaa53eda1cc902 Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Mon, 9 Oct 2023 00:16:22 +0400 Subject: [PATCH 9/9] Increase the timeout. --- tests/CrawlerProcess/sleeping.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/CrawlerProcess/sleeping.py b/tests/CrawlerProcess/sleeping.py index b46f7ee2d..420d9d328 100644 --- a/tests/CrawlerProcess/sleeping.py +++ b/tests/CrawlerProcess/sleeping.py @@ -14,7 +14,7 @@ class SleepingSpider(scrapy.Spider): from twisted.internet import reactor d = Deferred() - reactor.callLater(2, d.callback, None) + reactor.callLater(3, d.callback, None) await maybe_deferred_to_future(d)