Repository: scrapy/scrapy
Pull request #1423 (2015-08-11 07:32:40)

[MRG+1] Add ExecutionEngine.close() method
Depending on what state the engine is in, there are different measures that need to be taken to shut it down gracefully (i.e. leaving the reactor clean): Stop the engine, close the spider, or close the downloader.

This PR adds a new method as a single entry point for shutting down the engine and integrates it into `Crawler.crawl()` for graceful error handling during the crawling process.

curita: :+1:

Some other proposed solutions can be seen here: https://github.com/scrapy/scrapy/pull/1272#issuecomment-121534624, but this one seems the most practical.
codecov-io: ## [Current coverage][1] is `82.65%`
> Merging **#1423** into **master** will increase coverage by **+0.03%** as of [`b424aad`][3]



[1]: https://codecov.io/github/scrapy/scrapy?ref=b424aadac196e251682b13582eb9d16178918aa3
[2]: https://codecov.io/github/scrapy/scrapy/features/suggestions?ref=b424aadac196e251682b13582eb9d16178918aa3
[3]: https://codecov.io/github/scrapy/scrapy/commit/b424aadac196e251682b13582eb9d16178918aa3
[4]: https://codecov.io/github/scrapy/scrapy/compare/4a9f76ebd3242b2f91e238c26ff38874e9ee1960...b424aadac196e251682b13582eb9d16178918aa3

> Powered by [Codecov](https://codecov.io). Updated on successful CI builds.
jdemaeyer: Thanks for the feedback everybody, and apologies for the huge delay :(

I've implemented @nramirezuy's solution to keep the traceback and got rid of the mock in the test (thanks @kmike)
jdemaeyer: Hm I don't see why codecov marks the `yield exc` line as 'not hit'. The test fails when I remove it and passes when it's there.
kmike: @jdemaeyer it may be a codecov issue similar to https://github.com/codecov/support/issues/100
jdemaeyer: After chatting back and forth with Steve from Codecov, and doing [some more tests](https://codecov.io/github/scrapy/scrapy/scrapy/crawler.py?ref=6f10968f1a3baed94bc1d5d5419c7161f20320c3#l-69) to make sure that the `yield` line is indeed hit, we came to the conclusion that it is an issue with coverage.py.

I've reported it to them and Ned is currently looking into it (after mentioning that "BTW: I used scrapy yesterday, good stuff :)"). So I think the coverage should be fine.

Patch:
diff --git a/scrapy/core/engine.py b/scrapy/core/engine.py
index 992327bfe..eb2779b12 100644
--- a/scrapy/core/engine.py
+++ b/scrapy/core/engine.py
@@ -84,6 +84,21 @@ class ExecutionEngine(object):
         dfd = self._close_all_spiders()
         return dfd.addBoth(lambda _: self._finish_stopping_engine())
 
+    def close(self):
+        """Close the execution engine gracefully.
+
+        If it has already been started, stop it. In all cases, close all spiders
+        and the downloader.
+        """
+        if self.running:
+            # Will also close spiders and downloader
+            return self.stop()
+        elif self.open_spiders:
+            # Will also close downloader
+            return self._close_all_spiders()
+        else:
+            return defer.succeed(self.downloader.close())
+
     def pause(self):
         """Pause the execution engine"""
         self.paused = True
diff --git a/scrapy/crawler.py b/scrapy/crawler.py
index c7e3bb528..a1937f2bd 100644
--- a/scrapy/crawler.py
+++ b/scrapy/crawler.py
@@ -73,8 +73,11 @@ class Crawler(object):
             yield self.engine.open_spider(self.spider, start_requests)
             yield defer.maybeDeferred(self.engine.start)
         except Exception:
+            exc = defer.fail()
             self.crawling = False
-            raise
+            if self.engine is not None:
+                yield self.engine.close()
+            yield exc
 
     def _create_spider(self, *args, **kwargs):
         return self.spidercls.from_crawler(self, *args, **kwargs)


Test patch:
diff --git a/tests/test_crawl.py b/tests/test_crawl.py
index 6d21acab0..b2105dcfa 100644
--- a/tests/test_crawl.py
+++ b/tests/test_crawl.py
@@ -226,3 +226,24 @@ with multiples lines
         s = dict(est[0])
         self.assertEqual(s['engine.spider.name'], crawler.spider.name)
         self.assertEqual(s['len(engine.scraper.slot.active)'], 1)
+
+    @defer.inlineCallbacks
+    def test_graceful_crawl_error_handling(self):
+        """
+        Test whether errors happening anywhere in Crawler.crawl() are properly
+        reported (and not somehow swallowed) after a graceful engine shutdown.
+        The errors should not come from within Scrapy's core but from within
+        spiders/middlewares/etc., e.g. raised in Spider.start_requests(),
+        SpiderMiddleware.process_start_requests(), etc.
+        """
+
+        class TestError(Exception):
+            pass
+
+        class FaultySpider(SimpleSpider):
+            def start_requests(self):
+                raise TestError
+
+        crawler = get_crawler(FaultySpider)
+        yield self.assertFailure(crawler.crawl(), TestError)
+        self.assertFalse(crawler.crawling)
diff --git a/tests/test_engine.py b/tests/test_engine.py
index e14957eae..dad921a60 100644
--- a/tests/test_engine.py
+++ b/tests/test_engine.py
@@ -19,6 +19,7 @@ from twisted.web import server, static, util
 from twisted.trial import unittest
 
 from scrapy import signals
+from scrapy.core.engine import ExecutionEngine
 from scrapy.utils.test import get_crawler
 from pydispatch import dispatcher
 from tests import tests_datadir
@@ -234,6 +235,29 @@ class EngineTest(unittest.TestCase):
         self.assertEqual({'spider': self.run.spider, 'reason': 'finished'},
                          self.run.signals_catched[signals.spider_closed])
 
+    @defer.inlineCallbacks
+    def test_close_downloader(self):
+        e = ExecutionEngine(get_crawler(TestSpider), lambda: None)
+        yield e.close()
+
+    @defer.inlineCallbacks
+    def test_close_spiders_downloader(self):
+        e = ExecutionEngine(get_crawler(TestSpider), lambda: None)
+        yield e.open_spider(TestSpider(), [])
+        self.assertEqual(len(e.open_spiders), 1)
+        yield e.close()
+        self.assertEqual(len(e.open_spiders), 0)
+
+    @defer.inlineCallbacks
+    def test_close_engine_spiders_downloader(self):
+        e = ExecutionEngine(get_crawler(TestSpider), lambda: None)
+        yield e.open_spider(TestSpider(), [])
+        e.start()
+        self.assertTrue(e.running)
+        yield e.close()
+        self.assertFalse(e.running)
+        self.assertEqual(len(e.open_spiders), 0)
+
 
 if __name__ == "__main__":
     if len(sys.argv) > 1 and sys.argv[1] == 'runserver':