Fix start_requests lazyness and early hangs

- Removes new public methods added by scrapy#330 to Crawler and CrawlerProcess
- Add test for start_requests lazy evaluation
- Fix and test hangs when start_requests erroed before returning the generator
- Add test when start_requests fails while generating requests
- Simplify Crawler and CrawlerProcess implementation taking in count
  that only one spider can be attached per Crawler. As required by SEP-019
- "scrapy settings" command do not require starting a Crawler anymore

Author: dangra

scrapy/commands/settings.py

@@ -25,9 +25,7 @@ def add_options(self, parser):
             help="print setting value, intepreted as an float")
 
     def run(self, args, opts):
-        crawler = self.crawler_process.create_crawler()
-        crawler.configure()
-        settings = crawler.settings
+        settings = self.crawler_process.settings
         if opts.get:
             print settings.get(opts.get)
         elif opts.getbool:

scrapy/crawler.py

@@ -19,17 +19,20 @@ def __init__(self, settings):
         self.settings = settings
         self.signals = SignalManager(self)
         self.stats = load_object(settings['STATS_CLASS'])(self)
-
+        self._start_requests = lambda: ()
+        self._spider = None
+        # TODO: move SpiderManager to CrawlerProcess
         spman_cls = load_object(self.settings['SPIDER_MANAGER_CLASS'])
         self.spiders = spman_cls.from_crawler(self)
-        self._scheduled = {}
 
     def install(self):
+        # TODO: remove together with scrapy.project.crawler usage
         import scrapy.project
         assert not hasattr(scrapy.project, 'crawler'), "crawler already installed"
         scrapy.project.crawler = self
 
     def uninstall(self):
+        # TODO: remove together with scrapy.project.crawler usage
         import scrapy.project
         assert hasattr(scrapy.project, 'crawler'), "crawler not installed"
         del scrapy.project.crawler
@@ -45,19 +48,13 @@ def configure(self):
         self.engine = ExecutionEngine(self, self._spider_closed)
 
     def crawl(self, spider, requests=None):
+        assert self._spider is None, 'Spider already attached'
+        self._spider = spider
         spider.set_crawler(self)
-        if self.configured and self.engine.running:
-            assert not self._scheduled
-            return self._schedule(spider, requests)
-        elif requests is None:
-            self._scheduled[spider] = None
+        if requests is None:
+            self._start_requests = spider.start_requests
         else:
-            self._scheduled.setdefault(spider, []).append(requests)
-
-    def _schedule(self, spider, batches=()):
-        requests = chain.from_iterable(batches) \
-            if batches else spider.start_requests()
-        return self.engine.open_spider(spider, requests)
+            self._start_requests = lambda: requests
 
     def _spider_closed(self, spider=None):
         if not self.engine.open_spiders:
@@ -66,47 +63,40 @@ def _spider_closed(self, spider=None):
     @defer.inlineCallbacks
     def start(self):
         yield defer.maybeDeferred(self.configure)
-
-        for spider, batches in self._scheduled.iteritems():
-            yield self._schedule(spider, batches)
-
+        if self._spider:
+            yield self.engine.open_spider(self._spider, self._start_requests())
         yield defer.maybeDeferred(self.engine.start)
 
     @defer.inlineCallbacks
     def stop(self):
-        if self.engine.running:
+        if self.configured and self.engine.running:
             yield defer.maybeDeferred(self.engine.stop)
 
 
-class ProcessMixin(object):
-    """ Mixin which provides automatic control of the Twisted reactor and
-        installs some convenient signals for shutting it down
-    """
+class CrawlerProcess(object):
+    """ A class to run multiple scrapy crawlers in a process sequentially"""
 
-    def __init__(self, *a, **kw):
+    def __init__(self, settings):
         install_shutdown_handlers(self._signal_shutdown)
+        self.settings = settings
+        self.crawlers = {}
+        self.stopping = False
+
+    def create_crawler(self, name=None):
+        if name not in self.crawlers:
+            self.crawlers[name] = Crawler(self.settings)
+
+        return self.crawlers[name]
 
     def start(self):
         if self.start_crawling():
             self.start_reactor()
 
-    def start_reactor(self):
-        if self.settings.getbool('DNSCACHE_ENABLED'):
-            reactor.installResolver(CachingThreadedResolver(reactor))
-        reactor.addSystemEventTrigger('before', 'shutdown', self.stop)
-        reactor.run(installSignalHandlers=False)  # blocking call
-
-    def start_crawling(self):
-        raise NotImplementedError
-
+    @defer.inlineCallbacks
     def stop(self):
-        raise NotImplementedError
-
-    def stop_reactor(self, _=None):
-        try:
-            reactor.stop()
-        except RuntimeError:  # raised if already stopped or in shutdown stage
-            pass
+        self.stopping = True
+        for crawler in self.crawlers.itervalues():
+            yield crawler.stop()
 
     def _signal_shutdown(self, signum, _):
         install_shutdown_handlers(self._signal_kill)
@@ -120,27 +110,26 @@ def _signal_kill(self, signum, _):
         signame = signal_names[signum]
         log.msg(format='Received %(signame)s twice, forcing unclean shutdown',
                 level=log.INFO, signame=signame)
-        reactor.callFromThread(self.stop_reactor)
-
-
-class CrawlerProcess(ProcessMixin):
-    """ A class to run multiple scrapy crawlers in a process sequentially
-    """
-
-    def __init__(self, settings):
-        super(CrawlerProcess, self).__init__(settings)
-
-        self.settings = settings
-        self.crawlers = {}
-        self.stopping = False
-
-    def create_crawler(self, name=None):
-        if name not in self.crawlers:
-            self.crawlers[name] = Crawler(self.settings)
+        reactor.callFromThread(self._stop_reactor)
+
+    # ------------------------------------------------------------------------#
+    # The following public methods can't be considered stable and may change at
+    # any moment.
+    #
+    # start_crawling and start_reactor are called from scrapy.commands.shell
+    # They are splitted because reactor is started on a different thread than IPython shell.
+    #
+    def start_crawling(self):
+        log.scrapy_info(self.settings)
+        return self._start_crawler() is not None
 
-        return self.crawlers[name]
+    def start_reactor(self):
+        if self.settings.getbool('DNSCACHE_ENABLED'):
+            reactor.installResolver(CachingThreadedResolver(reactor))
+        reactor.addSystemEventTrigger('before', 'shutdown', self.stop)
+        reactor.run(installSignalHandlers=False)  # blocking call
 
-    def start_crawler(self):
+    def _start_crawler(self):
         if self.crawlers and not self.stopping:
             name, crawler = self.crawlers.popitem()
 
@@ -151,23 +140,17 @@ def start_crawler(self):
             if sflo:
                 crawler.signals.connect(sflo.stop, signals.engine_stopped)
 
-            crawler.signals.connect(self.check_done, signals.engine_stopped)
+            crawler.signals.connect(self._check_done, signals.engine_stopped)
             crawler.start()
 
             return name, crawler
 
-    def check_done(self, **kwargs):
-        if not self.start_crawler():
-            self.stop_reactor()
-
-    def start_crawling(self):
-        log.scrapy_info(self.settings)
-        return self.start_crawler() is not None
-
-    @defer.inlineCallbacks
-    def stop(self):
-        self.stopping = True
+    def _check_done(self, **kwargs):
+        if not self._start_crawler():
+            self._stop_reactor()
 
-        for crawler in self.crawlers.itervalues():
-            if crawler.configured:
-                yield crawler.stop()
+    def _stop_reactor(self, _=None):
+        try:
+            reactor.stop()
+        except RuntimeError:  # raised if already stopped or in shutdown stage
+            pass

scrapy/tests/spiders.py

@@ -103,3 +103,32 @@ def parse(self, response):
         for request in super(ErrorSpider, self).parse(response):
             yield request
             self.raise_exception()
+
+
+class BrokenStartRequestsSpider(FollowAllSpider):
+
+    fail_before_yield = False
+    fail_yielding = False
+
+    def __init__(self, *a, **kw):
+        super(BrokenStartRequestsSpider, self).__init__(*a, **kw)
+        self.seedsseen = []
+
+    def start_requests(self):
+        if self.fail_before_yield:
+            1 / 0
+
+        for s in xrange(100):
+            qargs = {'total': 10, 'seed': s}
+            url = "http://localhost:8998/follow?%s" % urlencode(qargs, doseq=1)
+            yield Request(url, meta={'seed': s})
+            if self.fail_yielding:
+                2 / 0
+
+        assert self.seedsseen, \
+                'All start requests consumed before any download happened'
+
+    def parse(self, response):
+        self.seedsseen.append(response.meta.get('seed'))
+        for req in super(BrokenStartRequestsSpider, self).parse(response):
+            yield req

scrapy/tests/test_cmdline/__init__.py

@@ -18,14 +18,14 @@ def _execute(self, *new_args, **kwargs):
 
     def test_default_settings(self):
         self.assertEqual(self._execute('settings', '--get', 'TEST1'), \
-            'default + started')
+                         'default')
 
     def test_override_settings_using_set_arg(self):
         self.assertEqual(self._execute('settings', '--get', 'TEST1', '-s', 'TEST1=override'), \
-            'override + started')
+                         'override')
 
     def test_override_settings_using_envvar(self):
         self.env['SCRAPY_TEST1'] = 'override'
         self.assertEqual(self._execute('settings', '--get', 'TEST1'), \
-            'override + started')
+                         'override')
 

scrapy/tests/test_crawl.py

@@ -1,13 +1,13 @@
 from twisted.internet import defer
 from twisted.trial.unittest import TestCase
 from scrapy.utils.test import get_crawler, get_testlog
-from scrapy.tests.spiders import FollowAllSpider, DelaySpider, SimpleSpider
+from scrapy.tests.spiders import FollowAllSpider, DelaySpider, SimpleSpider, \
+    BrokenStartRequestsSpider
 from scrapy.tests.mockserver import MockServer
 
 
 def docrawl(spider, settings=None):
     crawler = get_crawler(settings)
-    crawler.configure()
     crawler.crawl(spider)
     return crawler.start()
 
@@ -90,6 +90,29 @@ def test_retry_dns_error(self):
         yield docrawl(spider)
         self._assert_retried()
 
+    @defer.inlineCallbacks
+    def test_start_requests_bug_before_yield(self):
+        spider = BrokenStartRequestsSpider(fail_before_yield=1)
+        yield docrawl(spider)
+        errors = self.flushLoggedErrors(ZeroDivisionError)
+        self.assertEqual(len(errors), 1)
+
+    @defer.inlineCallbacks
+    def test_start_requests_bug_yielding(self):
+        spider = BrokenStartRequestsSpider(fail_yielding=1)
+        yield docrawl(spider)
+        errors = self.flushLoggedErrors(ZeroDivisionError)
+        self.assertEqual(len(errors), 1)
+
+    @defer.inlineCallbacks
+    def test_start_requests_lazyness(self):
+        settings = {"CONCURRENT_REQUESTS": 1}
+        spider = BrokenStartRequestsSpider()
+        yield docrawl(spider, settings)
+        #self.assertTrue(False, spider.seedsseen)
+        #self.assertTrue(spider.seedsseen.index(None) < spider.seedsseen.index(99),
+        #                spider.seedsseen)
+
     @defer.inlineCallbacks
     def test_unbounded_response(self):
         # Completeness of responses without Content-Length or Transfer-Encoding