Always check robots.txt before making another request in RobotsTxtMiddleware.

docs/topics/downloader-middleware.rst

@@ -879,12 +879,6 @@ RobotsTxtMiddleware
     To make sure Scrapy respects robots.txt make sure the middleware is enabled
     and the :setting:`ROBOTSTXT_OBEY` setting is enabled.
 
-    .. warning:: Keep in mind that, if you crawl using multiple concurrent
-       requests per domain, Scrapy could still download some forbidden pages
-       if they were requested before the robots.txt file was downloaded. This
-       is a known limitation of the current robots.txt middleware and will
-       be fixed in the future.
-
 .. reqmeta:: dont_obey_robotstxt
 
 If :attr:`Request.meta <scrapy.http.Request.meta>` has

scrapy/downloadermiddlewares/robotstxt.py

@@ -8,6 +8,7 @@ import logging
 
 from six.moves.urllib import robotparser
 
+from twisted.internet.defer import Deferred, maybeDeferred
 from scrapy.exceptions import NotConfigured, IgnoreRequest
 from scrapy.http import Request
 from scrapy.utils.httpobj import urlparse_cached
@@ -34,17 +35,22 @@ class RobotsTxtMiddleware(object):
     def process_request(self, request, spider):
         if request.meta.get('dont_obey_robotstxt'):
             return
-        rp = self.robot_parser(request, spider)
-        if rp and not rp.can_fetch(self._useragent, request.url):
+        d = maybeDeferred(self.robot_parser, request, spider)
+        d.addCallback(self.process_request_2, request, spider)
+        return d
+
+    def process_request_2(self, rp, request, spider):
+        if rp is not None and not rp.can_fetch(self._useragent, request.url):
             logger.debug("Forbidden by robots.txt: %(request)s",
                          {'request': request}, extra={'spider': spider})
-            raise IgnoreRequest
+            raise IgnoreRequest()
 
     def robot_parser(self, request, spider):
         url = urlparse_cached(request)
         netloc = url.netloc
+
         if netloc not in self._parsers:
-            self._parsers[netloc] = None
+            self._parsers[netloc] = Deferred()
             robotsurl = "%s://%s/robots.txt" % (url.scheme, url.netloc)
             robotsreq = Request(
                 robotsurl,
@@ -52,9 +58,19 @@ class RobotsTxtMiddleware(object):
                 meta={'dont_obey_robotstxt': True}
             )
             dfd = self.crawler.engine.download(robotsreq, spider)
-            dfd.addCallback(self._parse_robots)
+            dfd.addCallback(self._parse_robots, netloc)
             dfd.addErrback(self._logerror, robotsreq, spider)
-        return self._parsers[netloc]
+            dfd.addErrback(self._robots_error, netloc)
+
+        if isinstance(self._parsers[netloc], Deferred):
+            d = Deferred()
+            def cb(result):
+                d.callback(result)
+                return result
+            self._parsers[netloc].addCallback(cb)
+            return d
+        else:
+            return self._parsers[netloc]
 
     def _logerror(self, failure, request, spider):
         if failure.type is not IgnoreRequest:
@@ -62,8 +78,9 @@ class RobotsTxtMiddleware(object):
                          {'request': request, 'f_exception': failure.value},
                          exc_info=failure_to_exc_info(failure),
                          extra={'spider': spider})
+        return failure
 
-    def _parse_robots(self, response):
+    def _parse_robots(self, response, netloc):
         rp = robotparser.RobotFileParser(response.url)
         body = ''
         if hasattr(response, 'body_as_unicode'):
@@ -78,4 +95,10 @@ class RobotsTxtMiddleware(object):
                 # 'disallow all' to 'allow any'.
                 pass
         rp.parse(body.splitlines())
-        self._parsers[urlparse_cached(response).netloc] = rp
+
+        rp_dfd = self._parsers[netloc]
+        self._parsers[netloc] = rp
+        rp_dfd.callback(rp)
+
+    def _robots_error(self, failure, netloc):
+        self._parsers.pop(netloc).callback(None)

tests/test_downloadermiddleware_robotstxt.py

@@ -1,7 +1,7 @@
 from __future__ import absolute_import
 import re
 from twisted.internet import reactor, error
-from twisted.internet.defer import Deferred
+from twisted.internet.defer import Deferred, DeferredList, maybeDeferred
 from twisted.python import failure
 from twisted.trial import unittest
 from scrapy.downloadermiddlewares.robotstxt import RobotsTxtMiddleware
@@ -44,32 +44,20 @@ class RobotsTxtMiddlewareTest(unittest.TestCase):
 
     def test_robotstxt(self):
         middleware = RobotsTxtMiddleware(self._get_successful_crawler())
-        # There is a bit of neglect in robotstxt.py: robots.txt is fetched asynchronously,
-        # and it is actually fetched only *after* first process_request completes.
-        # So, first process_request will always succeed.
-        # We defer test() because otherwise robots.txt download mock will be called after assertRaises failure.
-        self.assertNotIgnored(Request('http://site.local'), middleware)
-        def test(r):
-            self.assertNotIgnored(Request('http://site.local/allowed'), middleware)
-            self.assertIgnored(Request('http://site.local/admin/main'), middleware)
+        return DeferredList([
+            self.assertNotIgnored(Request('http://site.local/allowed'), middleware),
+            self.assertIgnored(Request('http://site.local/admin/main'), middleware),
             self.assertIgnored(Request('http://site.local/static/'), middleware)
-        deferred = Deferred()
-        deferred.addCallback(test)
-        reactor.callFromThread(deferred.callback, None)
-        return deferred
+        ], fireOnOneErrback=True)
 
     def test_robotstxt_meta(self):
         middleware = RobotsTxtMiddleware(self._get_successful_crawler())
         meta = {'dont_obey_robotstxt': True}
-        self.assertNotIgnored(Request('http://site.local', meta=meta), middleware)
-        def test(r):
-            self.assertNotIgnored(Request('http://site.local/allowed', meta=meta), middleware)
-            self.assertNotIgnored(Request('http://site.local/admin/main', meta=meta), middleware)
+        return DeferredList([
+            self.assertNotIgnored(Request('http://site.local/allowed', meta=meta), middleware),
+            self.assertNotIgnored(Request('http://site.local/admin/main', meta=meta), middleware),
             self.assertNotIgnored(Request('http://site.local/static/', meta=meta), middleware)
-        deferred = Deferred()
-        deferred.addCallback(test)
-        reactor.callFromThread(deferred.callback, None)
-        return deferred
+        ], fireOnOneErrback=True)
 
     def _get_garbage_crawler(self):
         crawler = self.crawler
@@ -85,17 +73,12 @@ class RobotsTxtMiddlewareTest(unittest.TestCase):
     def test_robotstxt_garbage(self):
         # garbage response should be discarded, equal 'allow all'
         middleware = RobotsTxtMiddleware(self._get_garbage_crawler())
-        middleware._logerror = mock.MagicMock()
-        middleware.process_request(Request('http://site.local'), None)
-        self.assertNotIgnored(Request('http://site.local'), middleware)
-        def test(r):
-            self.assertNotIgnored(Request('http://site.local/allowed'), middleware)
-            self.assertNotIgnored(Request('http://site.local/admin/main'), middleware)
+        deferred = DeferredList([
+            self.assertNotIgnored(Request('http://site.local'), middleware),
+            self.assertNotIgnored(Request('http://site.local/allowed'), middleware),
+            self.assertNotIgnored(Request('http://site.local/admin/main'), middleware),
             self.assertNotIgnored(Request('http://site.local/static/'), middleware)
-        deferred = Deferred()
-        deferred.addCallback(test)
-        deferred.addErrback(lambda _: self.assertIsNone(middleware._logerror.assert_any_call()))
-        reactor.callFromThread(deferred.callback, None)
+        ], fireOnOneErrback=True)
         return deferred
 
     def _get_emptybody_crawler(self):
@@ -112,15 +95,11 @@ class RobotsTxtMiddlewareTest(unittest.TestCase):
     def test_robotstxt_empty_response(self):
         # empty response should equal 'allow all'
         middleware = RobotsTxtMiddleware(self._get_emptybody_crawler())
-        self.assertNotIgnored(Request('http://site.local'), middleware)
-        def test(r):
-            self.assertNotIgnored(Request('http://site.local/allowed'), middleware)
-            self.assertNotIgnored(Request('http://site.local/admin/main'), middleware)
+        return DeferredList([
+            self.assertNotIgnored(Request('http://site.local/allowed'), middleware),
+            self.assertNotIgnored(Request('http://site.local/admin/main'), middleware),
             self.assertNotIgnored(Request('http://site.local/static/'), middleware)
-        deferred = Deferred()
-        deferred.addCallback(test)
-        reactor.callFromThread(deferred.callback, None)
-        return deferred
+        ], fireOnOneErrback=True)
 
     def test_robotstxt_error(self):
         self.crawler.settings.set('ROBOTSTXT_OBEY', True)
@@ -132,17 +111,18 @@ class RobotsTxtMiddlewareTest(unittest.TestCase):
         self.crawler.engine.download.side_effect = return_failure
 
         middleware = RobotsTxtMiddleware(self.crawler)
-        middleware._logerror = mock.MagicMock()
-        middleware.process_request(Request('http://site.local'), None)
-        deferred = Deferred()
-        deferred.addErrback(lambda _: self.assertIsNone(middleware._logerror.assert_any_call()))
-        reactor.callFromThread(deferred.callback, None)
+        middleware._logerror = mock.MagicMock(side_effect=lambda fail, req, spider: fail)
+        deferred = middleware.process_request(Request('http://site.local'), None)
+        deferred.addCallback(lambda _: self.assertTrue(middleware._logerror.called))
         return deferred
 
     def assertNotIgnored(self, request, middleware):
         spider = None  # not actually used
-        self.assertIsNone(middleware.process_request(request, spider))
+        dfd = maybeDeferred(middleware.process_request, request, spider)
+        dfd.addCallback(self.assertIsNone)
+        return dfd
 
     def assertIgnored(self, request, middleware):
         spider = None  # not actually used
-        self.assertRaises(IgnoreRequest, middleware.process_request, request, spider)
+        return self.assertFailure(maybeDeferred(middleware.process_request, request, spider),
+                                  IgnoreRequest)