From 9ce9a293a6e0eefcb43f61d38d5f7ccc655d7889 Mon Sep 17 00:00:00 2001 From: Artur Gaspar Date: Tue, 1 Sep 2015 15:24:55 -0300 Subject: [PATCH] Always check robots.txt before making another request in RobotsTxtMiddleware. --- docs/topics/downloader-middleware.rst | 6 -- scrapy/downloadermiddlewares/robotstxt.py | 39 ++++++++--- tests/test_downloadermiddleware_robotstxt.py | 72 +++++++------------- 3 files changed, 57 insertions(+), 60 deletions(-) diff --git a/docs/topics/downloader-middleware.rst b/docs/topics/downloader-middleware.rst index 4603c555b..38c9456db 100644 --- a/docs/topics/downloader-middleware.rst +++ b/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 ` has diff --git a/scrapy/downloadermiddlewares/robotstxt.py b/scrapy/downloadermiddlewares/robotstxt.py index 457620d85..c061c2407 100644 --- a/scrapy/downloadermiddlewares/robotstxt.py +++ b/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) diff --git a/tests/test_downloadermiddleware_robotstxt.py b/tests/test_downloadermiddleware_robotstxt.py index b9c002f85..8a7238dd1 100644 --- a/tests/test_downloadermiddleware_robotstxt.py +++ b/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)