Repository: scrapy/scrapy
Pull request #3226 (2018-04-20 12:05:20)

[MRG+1] Addressing issue #3211
Return non-zero exit code from commands in case if we have error in spiders constructor

codecov[bot]: # [Codecov](https://codecov.io/gh/scrapy/scrapy/pull/3226?src=pr&el=h1) Report
> Merging [#3226](https://codecov.io/gh/scrapy/scrapy/pull/3226?src=pr&el=desc) into [master](https://codecov.io/gh/scrapy/scrapy/commit/da1256a9c8452703213c96ebc32fae1524034442?src=pr&el=desc) will **decrease** coverage by `0.39%`.
> The diff coverage is `81.81%`.


```diff
@@            Coverage Diff            @@
##           master    #3226     +/-   ##
=========================================
- Coverage   82.12%   81.72%   -0.4%     
=========================================
  Files         228      228             
  Lines        9593     9604     +11     
  Branches     1385     1388      +3     
=========================================
- Hits         7878     7849     -29     
- Misses       1456     1492     +36     
- Partials      259      263      +4
```

| [Impacted Files](https://codecov.io/gh/scrapy/scrapy/pull/3226?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [scrapy/commands/crawl.py](https://codecov.io/gh/scrapy/scrapy/pull/3226/diff?src=pr&el=tree#diff-c2NyYXB5L2NvbW1hbmRzL2NyYXdsLnB5) | `27.9% <0%> (-1.37%)` | :arrow_down: |
| [scrapy/crawler.py](https://codecov.io/gh/scrapy/scrapy/pull/3226/diff?src=pr&el=tree#diff-c2NyYXB5L2NyYXdsZXIucHk=) | `92.3% <100%> (+0.94%)` | :arrow_up: |
| [scrapy/commands/runspider.py](https://codecov.io/gh/scrapy/scrapy/pull/3226/diff?src=pr&el=tree#diff-c2NyYXB5L2NvbW1hbmRzL3J1bnNwaWRlci5weQ==) | `74.28% <100%> (+0.75%)` | :arrow_up: |
| [scrapy/core/downloader/handlers/s3.py](https://codecov.io/gh/scrapy/scrapy/pull/3226/diff?src=pr&el=tree#diff-c2NyYXB5L2NvcmUvZG93bmxvYWRlci9oYW5kbGVycy9zMy5weQ==) | `62.9% <0%> (-32.26%)` | :arrow_down: |
| [scrapy/utils/boto.py](https://codecov.io/gh/scrapy/scrapy/pull/3226/diff?src=pr&el=tree#diff-c2NyYXB5L3V0aWxzL2JvdG8ucHk=) | `46.66% <0%> (-26.67%)` | :arrow_down: |
| [scrapy/core/downloader/tls.py](https://codecov.io/gh/scrapy/scrapy/pull/3226/diff?src=pr&el=tree#diff-c2NyYXB5L2NvcmUvZG93bmxvYWRlci90bHMucHk=) | `77.5% <0%> (-12.5%)` | :arrow_down: |
| [scrapy/core/downloader/handlers/http11.py](https://codecov.io/gh/scrapy/scrapy/pull/3226/diff?src=pr&el=tree#diff-c2NyYXB5L2NvcmUvZG93bmxvYWRlci9oYW5kbGVycy9odHRwMTEucHk=) | `89.88% <0%> (-2.63%)` | :arrow_down: |
| [scrapy/pipelines/files.py](https://codecov.io/gh/scrapy/scrapy/pull/3226/diff?src=pr&el=tree#diff-c2NyYXB5L3BpcGVsaW5lcy9maWxlcy5weQ==) | `66.91% <0%> (-1.11%)` | :arrow_down: |

whalebot-helmsman: Test failed due to new twisted release, fix in twisted trunk https://github.com/twisted/twisted/pull/1008 but not released to pypy
kmike: Looks good to me, thanks @whalebot-helmsman! I'd remove `_is` from method names, but it is just a style preference. +1 to merge.

Patch:
diff --git a/docs/topics/leaks.rst b/docs/topics/leaks.rst
index 92590c180..af14d14e8 100644
--- a/docs/topics/leaks.rst
+++ b/docs/topics/leaks.rst
@@ -202,6 +202,7 @@ memory leaks (Requests, Responses, Items, and Selectors). However, there are
 other cases where the memory leaks could come from other (more or less obscure)
 objects. If this is your case, and you can't find your leaks using ``trackref``,
 you still have another resource: the `Guppy library`_.
+If you're using Python3, see :ref:`topics-leaks-muppy`.
 
 .. _Guppy library: https://pypi.python.org/pypi/guppy
 
@@ -253,6 +254,50 @@ knowledge about Python internals. For more info about Guppy, refer to the
 
 .. _Guppy documentation: http://guppy-pe.sourceforge.net/
 
+.. _topics-leaks-muppy:
+
+Debugging memory leaks with muppy
+=================================
+If you're using Python 3, you can use muppy from `Pympler`_.
+
+.. _Pympler: https://pypi.org/project/Pympler/
+
+If you use ``pip``, you can install muppy with the following command::
+
+    pip install Pympler
+
+Here's an example to view all Python objects available in
+the heap using muppy::
+
+    >>> from pympler import muppy
+    >>> all_objects = muppy.get_objects()
+    >>> len(all_objects)
+    28667
+    >>> from pympler import summary
+    >>> suml = summary.summarize(all_objects)
+    >>> summary.print_(suml)
+                                   types |   # objects |   total size
+    ==================================== | =========== | ============
+                             <class 'str |        9822 |      1.10 MB
+                            <class 'dict |        1658 |    856.62 KB
+                            <class 'type |         436 |    443.60 KB
+                            <class 'code |        2974 |    419.56 KB
+              <class '_io.BufferedWriter |           2 |    256.34 KB
+                             <class 'set |         420 |    159.88 KB
+              <class '_io.BufferedReader |           1 |    128.17 KB
+              <class 'wrapper_descriptor |        1130 |     88.28 KB
+                           <class 'tuple |        1304 |     86.57 KB
+                         <class 'weakref |        1013 |     79.14 KB
+      <class 'builtin_function_or_method |         958 |     67.36 KB
+               <class 'method_descriptor |         865 |     60.82 KB
+                     <class 'abc.ABCMeta |          62 |     59.96 KB
+                            <class 'list |         446 |     58.52 KB
+                             <class 'int |        1425 |     43.20 KB
+
+For more info about muppy, refer to the `muppy documentation`_.
+
+.. _muppy documentation: https://pythonhosted.org/Pympler/muppy.html
+
 .. _topics-leaks-without-leaks:
 
 Leaks without leaks
diff --git a/scrapy/commands/crawl.py b/scrapy/commands/crawl.py
index 4b986bf9d..8093fd402 100644
--- a/scrapy/commands/crawl.py
+++ b/scrapy/commands/crawl.py
@@ -56,3 +56,6 @@ class Command(ScrapyCommand):
 
         self.crawler_process.crawl(spname, **opts.spargs)
         self.crawler_process.start()
+
+        if self.crawler_process.bootstrap_failed:
+            self.exitcode = 1
diff --git a/scrapy/commands/runspider.py b/scrapy/commands/runspider.py
index a98033dd1..376d3c84e 100644
--- a/scrapy/commands/runspider.py
+++ b/scrapy/commands/runspider.py
@@ -87,3 +87,6 @@ class Command(ScrapyCommand):
 
         self.crawler_process.crawl(spidercls, **opts.spargs)
         self.crawler_process.start()
+
+        if self.crawler_process.bootstrap_failed:
+            self.exitcode = 1
diff --git a/scrapy/crawler.py b/scrapy/crawler.py
index 5cbc2d7c5..04aee18ed 100644
--- a/scrapy/crawler.py
+++ b/scrapy/crawler.py
@@ -137,6 +137,7 @@ class CrawlerRunner(object):
         self.spider_loader = _get_spider_loader(settings)
         self._crawlers = set()
         self._active = set()
+        self.bootstrap_failed = False
 
     @property
     def spiders(self):
@@ -178,6 +179,7 @@ class CrawlerRunner(object):
         def _done(result):
             self.crawlers.discard(crawler)
             self._active.discard(d)
+            self.bootstrap_failed |= not getattr(crawler, 'spider', None)
             return result
 
         return d.addBoth(_done)


Test patch:
diff --git a/tests/test_commands.py b/tests/test_commands.py
index cb1301c95..7d9071b64 100644
--- a/tests/test_commands.py
+++ b/tests/test_commands.py
@@ -1,3 +1,4 @@
+import inspect
 import os
 import sys
 import subprocess
@@ -17,6 +18,7 @@ from scrapy.utils.python import retry_on_eintr
 from scrapy.utils.test import get_testenv
 from scrapy.utils.testsite import SiteTest
 from scrapy.utils.testproc import ProcessTest
+from tests.test_crawler import ExceptionSpider, NoRequestsSpider
 
 
 class ProjectTest(unittest.TestCase):
@@ -220,6 +222,16 @@ class MySpider(scrapy.Spider):
         self.assertIn("INFO: Closing spider (finished)", log)
         self.assertIn("INFO: Spider closed (finished)", log)
 
+    def test_run_fail_spider(self):
+        proc = self.runspider("import scrapy\n" + inspect.getsource(ExceptionSpider))
+        ret = proc.returncode
+        self.assertNotEqual(ret, 0)
+
+    def test_run_good_spider(self):
+        proc = self.runspider("import scrapy\n" + inspect.getsource(NoRequestsSpider))
+        ret = proc.returncode
+        self.assertEqual(ret, 0)
+
     def test_runspider_log_level(self):
         log = self.get_log(self.debug_log_spider,
                            args=('-s', 'LOG_LEVEL=INFO'))
diff --git a/tests/test_crawler.py b/tests/test_crawler.py
index ba0d709ff..d3b80f460 100644
--- a/tests/test_crawler.py
+++ b/tests/test_crawler.py
@@ -4,6 +4,9 @@ import tempfile
 import warnings
 import unittest
 
+from twisted.internet import defer
+import twisted.trial.unittest
+
 import scrapy
 from scrapy.crawler import Crawler, CrawlerRunner, CrawlerProcess
 from scrapy.settings import Settings, default_settings
@@ -11,9 +14,9 @@ from scrapy.spiderloader import SpiderLoader
 from scrapy.utils.log import configure_logging, get_scrapy_root_handler
 from scrapy.utils.spider import DefaultSpider
 from scrapy.utils.misc import load_object
+from scrapy.utils.test import get_crawler
 from scrapy.extensions.throttle import AutoThrottle
 
-
 class BaseCrawlerTest(unittest.TestCase):
 
     def assertOptionIsDefault(self, settings, key):
@@ -181,3 +184,62 @@ class CrawlerProcessTest(BaseCrawlerTest):
     def test_crawler_process_accepts_None(self):
         runner = CrawlerProcess()
         self.assertOptionIsDefault(runner.settings, 'RETRY_ENABLED')
+
+
+class ExceptionSpider(scrapy.Spider):
+    name = 'exception'
+
+    @classmethod
+    def from_crawler(cls, crawler, *args, **kwargs):
+        raise ValueError('Exception in from_crawler method')
+
+
+class NoRequestsSpider(scrapy.Spider):
+    name = 'no_request'
+
+    def start_requests(self):
+        return []
+
+
+class CrawlerRunnerHasSpider(twisted.trial.unittest.TestCase):
+
+    @defer.inlineCallbacks
+    def test_crawler_runner_bootstrap_successful(self):
+        runner = CrawlerRunner()
+        yield runner.crawl(NoRequestsSpider)
+        self.assertEqual(runner.bootstrap_failed, False)
+
+    @defer.inlineCallbacks
+    def test_crawler_runner_bootstrap_successful_for_several(self):
+        runner = CrawlerRunner()
+        yield runner.crawl(NoRequestsSpider)
+        yield runner.crawl(NoRequestsSpider)
+        self.assertEqual(runner.bootstrap_failed, False)
+
+    @defer.inlineCallbacks
+    def test_crawler_runner_bootstrap_failed(self):
+        runner = CrawlerRunner()
+
+        try:
+            yield runner.crawl(ExceptionSpider)
+        except ValueError:
+            pass
+        else:
+            self.fail('Exception should be raised from spider')
+
+        self.assertEqual(runner.bootstrap_failed, True)
+
+    @defer.inlineCallbacks
+    def test_crawler_runner_bootstrap_failed_for_several(self):
+        runner = CrawlerRunner()
+
+        try:
+            yield runner.crawl(ExceptionSpider)
+        except ValueError:
+            pass
+        else:
+            self.fail('Exception should be raised from spider')
+
+        yield runner.crawl(NoRequestsSpider)
+
+        self.assertEqual(runner.bootstrap_failed, True)