From 810658bcc5b1897d57c0882a0f7ab4a0d264a778 Mon Sep 17 00:00:00 2001 From: harshasrinivas Date: Sun, 12 Mar 2017 05:06:12 +0530 Subject: [PATCH 01/10] Add feature to set RETRY_TIMES per request (#2642) --- scrapy/downloadermiddlewares/retry.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scrapy/downloadermiddlewares/retry.py b/scrapy/downloadermiddlewares/retry.py index 549d74f46..a5342995f 100644 --- a/scrapy/downloadermiddlewares/retry.py +++ b/scrapy/downloadermiddlewares/retry.py @@ -63,6 +63,9 @@ class RetryMiddleware(object): def _retry(self, request, reason, spider): retries = request.meta.get('retry_times', 0) + 1 + if 'max_retry_times' in request.meta: + self.max_retry_times = request.meta['max_retry_times'] + stats = spider.crawler.stats if retries <= self.max_retry_times: logger.debug("Retrying %(request)s (failed %(retries)d times): %(reason)s", From 0d57b5cd43343a335fcf2e923b29e76d09dd0b51 Mon Sep 17 00:00:00 2001 From: harshasrinivas Date: Mon, 13 Mar 2017 02:10:23 +0530 Subject: [PATCH 02/10] Prevent max_retry_times override --- scrapy/downloadermiddlewares/retry.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/scrapy/downloadermiddlewares/retry.py b/scrapy/downloadermiddlewares/retry.py index a5342995f..07e979628 100644 --- a/scrapy/downloadermiddlewares/retry.py +++ b/scrapy/downloadermiddlewares/retry.py @@ -63,11 +63,13 @@ class RetryMiddleware(object): def _retry(self, request, reason, spider): retries = request.meta.get('retry_times', 0) + 1 + retry_times = self.max_retry_times + if 'max_retry_times' in request.meta: - self.max_retry_times = request.meta['max_retry_times'] + retry_times = request.meta['max_retry_times'] stats = spider.crawler.stats - if retries <= self.max_retry_times: + if retries <= retry_times: logger.debug("Retrying %(request)s (failed %(retries)d times): %(reason)s", {'request': request, 'retries': retries, 'reason': reason}, extra={'spider': spider}) From 694c6d3d7460ab80ace979c4563af8f310614d37 Mon Sep 17 00:00:00 2001 From: harshasrinivas Date: Tue, 14 Mar 2017 16:14:40 +0530 Subject: [PATCH 03/10] Simplify retry_times assignment statement --- scrapy/downloadermiddlewares/retry.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/scrapy/downloadermiddlewares/retry.py b/scrapy/downloadermiddlewares/retry.py index 07e979628..c22437ff1 100644 --- a/scrapy/downloadermiddlewares/retry.py +++ b/scrapy/downloadermiddlewares/retry.py @@ -63,10 +63,7 @@ class RetryMiddleware(object): def _retry(self, request, reason, spider): retries = request.meta.get('retry_times', 0) + 1 - retry_times = self.max_retry_times - - if 'max_retry_times' in request.meta: - retry_times = request.meta['max_retry_times'] + retry_times = request.meta.get('max_retry_times') or self.max_retry_times stats = spider.crawler.stats if retries <= retry_times: From 966bd49c421fdf40a8b21b49c76cd54ded06fe50 Mon Sep 17 00:00:00 2001 From: harshasrinivas Date: Tue, 14 Mar 2017 16:23:47 +0530 Subject: [PATCH 04/10] Update unittest for meta['max_retry_times'] --- tests/test_downloadermiddleware_retry.py | 28 ++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/test_downloadermiddleware_retry.py b/tests/test_downloadermiddleware_retry.py index b833cb448..cc3d37075 100644 --- a/tests/test_downloadermiddleware_retry.py +++ b/tests/test_downloadermiddleware_retry.py @@ -103,6 +103,34 @@ class RetryTest(unittest.TestCase): req = self.mw.process_exception(req, exception, self.spider) self.assertEqual(req, None) + def test_different_retry(self): + + req = Request('http://www.scrapytest.org/invalid_url', meta={'max_retry_times': 1}) + self._test_retry(req, DNSLookupError('foo')) + req2 = Request('http://www.scrapytest.org/invalid_url') + self._test_retry(req2, DNSLookupError('foo')) + + stats = self.crawler.stats + assert stats.get_value('retry/max_reached') == 2 + assert stats.get_value('retry/count') == 3 + + def _test_retry(self, req, exception): + + req = self.mw.process_exception(req, exception, self.spider) + assert isinstance(req, Request) + + retry_times = req.meta.get('max_retry_times') or self.mw.max_retry_times + + while req.meta['retry_times'] != retry_times: + req = self.mw.process_exception(req, exception, self.spider) + assert isinstance(req, Request) + + self.assertEqual(req.meta['retry_times'], retry_times) + + # discard it + req = self.mw.process_exception(req, exception, self.spider) + self.assertEqual(req, None) + if __name__ == "__main__": unittest.main() From e321ac9931007360a38dbd6a5933794af415274b Mon Sep 17 00:00:00 2001 From: harshasrinivas Date: Wed, 15 Mar 2017 04:12:32 +0530 Subject: [PATCH 05/10] Update unittests for max_retry_times --- tests/test_downloadermiddleware_retry.py | 37 ++++++++++++++---------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/tests/test_downloadermiddleware_retry.py b/tests/test_downloadermiddleware_retry.py index cc3d37075..064c740c9 100644 --- a/tests/test_downloadermiddleware_retry.py +++ b/tests/test_downloadermiddleware_retry.py @@ -105,27 +105,34 @@ class RetryTest(unittest.TestCase): def test_different_retry(self): - req = Request('http://www.scrapytest.org/invalid_url', meta={'max_retry_times': 1}) - self._test_retry(req, DNSLookupError('foo')) + req = Request('http://www.scrapytest.org/invalid_url', meta={'max_retry_times': 3}) + + # SETINGS: meta(max_retry_times) = 3, RETRY_TIMES = 2 + self._test_retry(req, DNSLookupError('foo'), 3) + req2 = Request('http://www.scrapytest.org/invalid_url') - self._test_retry(req2, DNSLookupError('foo')) - - stats = self.crawler.stats - assert stats.get_value('retry/max_reached') == 2 - assert stats.get_value('retry/count') == 3 - - def _test_retry(self, req, exception): - req = self.mw.process_exception(req, exception, self.spider) - assert isinstance(req, Request) + # SETINGS: RETRY_TIMES < meta(max_retry_times) + self._test_retry(req2, DNSLookupError('foo'), 2) - retry_times = req.meta.get('max_retry_times') or self.mw.max_retry_times + # SETINGS: RETRY_TIMES = 0 + self.mw.max_retry_times = 0 + self._test_retry(req2, DNSLookupError('foo'), 0) + + # SETINGS: RETRY_TIMES > meta(max_retry_times) + self.mw.max_retry_times = 4 + self._test_retry(req2, DNSLookupError('foo'), 4) - while req.meta['retry_times'] != retry_times: + # RESET RETRY_TIMES SETTINGS + self.mw.max_retry_times = 2 + + def _test_retry(self, req, exception, max_retry_times): + + while max_retry_times > 0: req = self.mw.process_exception(req, exception, self.spider) assert isinstance(req, Request) - - self.assertEqual(req.meta['retry_times'], retry_times) + if req.meta['retry_times'] == max_retry_times: + break # discard it req = self.mw.process_exception(req, exception, self.spider) From 9d97d788c06c2e8fbbdfbcfbee65321ac2dfb517 Mon Sep 17 00:00:00 2001 From: harshasrinivas Date: Wed, 15 Mar 2017 04:13:47 +0530 Subject: [PATCH 06/10] Update docs for meta key --- docs/topics/downloader-middleware.rst | 5 +++++ docs/topics/request-response.rst | 8 ++++++++ 2 files changed, 13 insertions(+) diff --git a/docs/topics/downloader-middleware.rst b/docs/topics/downloader-middleware.rst index c3a454279..b808a6448 100644 --- a/docs/topics/downloader-middleware.rst +++ b/docs/topics/downloader-middleware.rst @@ -852,6 +852,11 @@ Default: ``2`` Maximum number of times to retry, in addition to the first download. +.. reqmeta:: max_retry_times + +If :attr:`Request.meta ` has ``max_retry_times`` key +set to some value, this setting will be ignored by this middleware for the corresponding request. + .. setting:: RETRY_HTTP_CODES RETRY_HTTP_CODES diff --git a/docs/topics/request-response.rst b/docs/topics/request-response.rst index 67f8ec285..64a1e55fa 100644 --- a/docs/topics/request-response.rst +++ b/docs/topics/request-response.rst @@ -308,6 +308,7 @@ Those are: * ``ftp_user`` (See :setting:`FTP_USER` for more info) * ``ftp_password`` (See :setting:`FTP_PASSWORD` for more info) * :reqmeta:`referrer_policy` +* :reqmeta:`max_retry_times` .. reqmeta:: bindaddress @@ -342,6 +343,13 @@ download_fail_on_dataloss Whether or not to fail on broken responses. See: :setting:`DOWNLOAD_FAIL_ON_DATALOSS`. +.. reqmeta:: max_retry_times + +max_retry_times +--------------- + +The meta key is used set retry times per request. When initialized, the :setting:`RETRY_TIMES` setting will be ignored by the downloader middleware. + .. _topics-request-response-ref-request-subclasses: Request subclasses From 49c5afc5ff6c810c78678ea1c86beb19ca3487ad Mon Sep 17 00:00:00 2001 From: harshasrinivas Date: Sun, 19 Mar 2017 06:08:35 +0530 Subject: [PATCH 07/10] Fix bug involving OR condition --- scrapy/downloadermiddlewares/retry.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/scrapy/downloadermiddlewares/retry.py b/scrapy/downloadermiddlewares/retry.py index c22437ff1..07e979628 100644 --- a/scrapy/downloadermiddlewares/retry.py +++ b/scrapy/downloadermiddlewares/retry.py @@ -63,7 +63,10 @@ class RetryMiddleware(object): def _retry(self, request, reason, spider): retries = request.meta.get('retry_times', 0) + 1 - retry_times = request.meta.get('max_retry_times') or self.max_retry_times + retry_times = self.max_retry_times + + if 'max_retry_times' in request.meta: + retry_times = request.meta['max_retry_times'] stats = spider.crawler.stats if retries <= retry_times: From 0d9ebd6e1ed58654ea1996d5a236b4d0240590df Mon Sep 17 00:00:00 2001 From: harshasrinivas Date: Sun, 19 Mar 2017 06:15:46 +0530 Subject: [PATCH 08/10] Update tests for max_retry_times --- tests/test_downloadermiddleware_retry.py | 76 +++++++++++++++++++----- 1 file changed, 61 insertions(+), 15 deletions(-) diff --git a/tests/test_downloadermiddleware_retry.py b/tests/test_downloadermiddleware_retry.py index 064c740c9..5f1760fd1 100644 --- a/tests/test_downloadermiddleware_retry.py +++ b/tests/test_downloadermiddleware_retry.py @@ -103,29 +103,75 @@ class RetryTest(unittest.TestCase): req = self.mw.process_exception(req, exception, self.spider) self.assertEqual(req, None) - def test_different_retry(self): + +class MaxRetryTimesTest(unittest.TestCase): + def setUp(self): + self.crawler = get_crawler(Spider) + self.spider = self.crawler._create_spider('foo') + self.mw = RetryMiddleware.from_crawler(self.crawler) + self.mw.max_retry_times = 2 + + def test_without_metakey(self): - req = Request('http://www.scrapytest.org/invalid_url', meta={'max_retry_times': 3}) + req = Request('http://www.scrapytest.org/invalid_url') - # SETINGS: meta(max_retry_times) = 3, RETRY_TIMES = 2 - self._test_retry(req, DNSLookupError('foo'), 3) + # SETTINGS: RETRY_TIMES is NON-ZERO + self.mw.max_retry_times = 5 + self._test_retry(req, DNSLookupError('foo'), 5) - req2 = Request('http://www.scrapytest.org/invalid_url') - - # SETINGS: RETRY_TIMES < meta(max_retry_times) - self._test_retry(req2, DNSLookupError('foo'), 2) - - # SETINGS: RETRY_TIMES = 0 + # SETTINGS: RETRY_TIMES = 0 self.mw.max_retry_times = 0 - self._test_retry(req2, DNSLookupError('foo'), 0) - - # SETINGS: RETRY_TIMES > meta(max_retry_times) - self.mw.max_retry_times = 4 - self._test_retry(req2, DNSLookupError('foo'), 4) + self._test_retry(req, DNSLookupError('foo'), 0) # RESET RETRY_TIMES SETTINGS self.mw.max_retry_times = 2 + def test_with_metakey_preceding(self): + # request with meta(max_retry_times) is called first + + req1 = Request('http://www.scrapytest.org/invalid_url', meta={'max_retry_times': 3}) + req2 = Request('http://www.scrapytest.org/invalid_url') + req3 = Request('http://www.scrapytest.org/invalid_url', meta={'max_retry_times': 4}) + + # SETINGS: RETRY_TIMES < meta(max_retry_times) + self.mw.max_retry_times = 2 + self._test_retry(req1, DNSLookupError('foo'), 3) + self._test_retry(req2, DNSLookupError('foo'), 2) + + # SETINGS: RETRY_TIMES > meta(max_retry_times) + self.mw.max_retry_times = 5 + self._test_retry(req3, DNSLookupError('foo'), 4) + self._test_retry(req2, DNSLookupError('foo'), 5) + + # RESET RETRY_TIMES SETTINGS + self.mw.max_retry_times = 2 + + def test_with_metakey_succeeding(self): + # request with meta(max_retry_times) is called second + + req1 = Request('http://www.scrapytest.org/invalid_url', meta={'max_retry_times': 3}) + req2 = Request('http://www.scrapytest.org/invalid_url') + req3 = Request('http://www.scrapytest.org/invalid_url', meta={'max_retry_times': 4}) + + # SETINGS: RETRY_TIMES < meta(max_retry_times) + self.mw.max_retry_times = 2 + self._test_retry(req2, DNSLookupError('foo'), 2) + self._test_retry(req1, DNSLookupError('foo'), 3) + + # SETINGS: RETRY_TIMES > meta(max_retry_times) + self.mw.max_retry_times = 5 + self._test_retry(req2, DNSLookupError('foo'), 5) + self._test_retry(req3, DNSLookupError('foo'), 4) + + # RESET RETRY_TIMES SETTINGS + self.mw.max_retry_times = 2 + + def test_with_metakey_zero(self): + + req = Request('http://www.scrapytest.org/invalid_url', meta={'max_retry_times': 0}) + self._test_retry(req, DNSLookupError('foo'), 0) + + def _test_retry(self, req, exception, max_retry_times): while max_retry_times > 0: From 10741aca720293a12dedda4d1872cf0604b49f0b Mon Sep 17 00:00:00 2001 From: harshasrinivas Date: Sun, 19 Mar 2017 06:17:28 +0530 Subject: [PATCH 09/10] Update docs - improve clarity --- docs/topics/downloader-middleware.rst | 8 ++++---- docs/topics/request-response.rst | 4 +++- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/topics/downloader-middleware.rst b/docs/topics/downloader-middleware.rst index b808a6448..0d168017f 100644 --- a/docs/topics/downloader-middleware.rst +++ b/docs/topics/downloader-middleware.rst @@ -852,10 +852,10 @@ Default: ``2`` Maximum number of times to retry, in addition to the first download. -.. reqmeta:: max_retry_times - -If :attr:`Request.meta ` has ``max_retry_times`` key -set to some value, this setting will be ignored by this middleware for the corresponding request. +Maximum number of retries can also be specified per-request using +:reqmeta:`max_retry_times` attribute of :attr:`Request.meta `. +When initialized, the :reqmeta:`max_retry_times` meta key takes higher +precedence over the :setting:`RETRY_TIMES` setting. .. setting:: RETRY_HTTP_CODES diff --git a/docs/topics/request-response.rst b/docs/topics/request-response.rst index 64a1e55fa..03918fd2d 100644 --- a/docs/topics/request-response.rst +++ b/docs/topics/request-response.rst @@ -348,7 +348,9 @@ Whether or not to fail on broken responses. See: max_retry_times --------------- -The meta key is used set retry times per request. When initialized, the :setting:`RETRY_TIMES` setting will be ignored by the downloader middleware. +The meta key is used set retry times per request. When initialized, the +:reqmeta:`max_retry_times` meta key takes higher precedence over the +:setting:`RETRY_TIMES` setting. .. _topics-request-response-ref-request-subclasses: From 38e6857c957ef023533128f054688152be223c87 Mon Sep 17 00:00:00 2001 From: harshasrinivas Date: Thu, 23 Mar 2017 19:45:04 +0530 Subject: [PATCH 10/10] Improvise the clarity of test cases --- tests/test_downloadermiddleware_retry.py | 105 +++++++++++------------ 1 file changed, 51 insertions(+), 54 deletions(-) diff --git a/tests/test_downloadermiddleware_retry.py b/tests/test_downloadermiddleware_retry.py index 5f1760fd1..51b79b6c3 100644 --- a/tests/test_downloadermiddleware_retry.py +++ b/tests/test_downloadermiddleware_retry.py @@ -110,75 +110,72 @@ class MaxRetryTimesTest(unittest.TestCase): self.spider = self.crawler._create_spider('foo') self.mw = RetryMiddleware.from_crawler(self.crawler) self.mw.max_retry_times = 2 + self.invalid_url = 'http://www.scrapytest.org/invalid_url' - def test_without_metakey(self): - - req = Request('http://www.scrapytest.org/invalid_url') - - # SETTINGS: RETRY_TIMES is NON-ZERO - self.mw.max_retry_times = 5 - self._test_retry(req, DNSLookupError('foo'), 5) + def test_with_settings_zero(self): # SETTINGS: RETRY_TIMES = 0 self.mw.max_retry_times = 0 - self._test_retry(req, DNSLookupError('foo'), 0) - # RESET RETRY_TIMES SETTINGS - self.mw.max_retry_times = 2 - - def test_with_metakey_preceding(self): - # request with meta(max_retry_times) is called first - - req1 = Request('http://www.scrapytest.org/invalid_url', meta={'max_retry_times': 3}) - req2 = Request('http://www.scrapytest.org/invalid_url') - req3 = Request('http://www.scrapytest.org/invalid_url', meta={'max_retry_times': 4}) - - # SETINGS: RETRY_TIMES < meta(max_retry_times) - self.mw.max_retry_times = 2 - self._test_retry(req1, DNSLookupError('foo'), 3) - self._test_retry(req2, DNSLookupError('foo'), 2) - - # SETINGS: RETRY_TIMES > meta(max_retry_times) - self.mw.max_retry_times = 5 - self._test_retry(req3, DNSLookupError('foo'), 4) - self._test_retry(req2, DNSLookupError('foo'), 5) - - # RESET RETRY_TIMES SETTINGS - self.mw.max_retry_times = 2 - - def test_with_metakey_succeeding(self): - # request with meta(max_retry_times) is called second - - req1 = Request('http://www.scrapytest.org/invalid_url', meta={'max_retry_times': 3}) - req2 = Request('http://www.scrapytest.org/invalid_url') - req3 = Request('http://www.scrapytest.org/invalid_url', meta={'max_retry_times': 4}) - - # SETINGS: RETRY_TIMES < meta(max_retry_times) - self.mw.max_retry_times = 2 - self._test_retry(req2, DNSLookupError('foo'), 2) - self._test_retry(req1, DNSLookupError('foo'), 3) - - # SETINGS: RETRY_TIMES > meta(max_retry_times) - self.mw.max_retry_times = 5 - self._test_retry(req2, DNSLookupError('foo'), 5) - self._test_retry(req3, DNSLookupError('foo'), 4) - - # RESET RETRY_TIMES SETTINGS - self.mw.max_retry_times = 2 + req = Request(self.invalid_url) + self._test_retry(req, DNSLookupError('foo'), self.mw.max_retry_times) def test_with_metakey_zero(self): + + # SETTINGS: meta(max_retry_times) = 0 + meta_max_retry_times = 0 - req = Request('http://www.scrapytest.org/invalid_url', meta={'max_retry_times': 0}) + req = Request(self.invalid_url, meta={'max_retry_times': meta_max_retry_times}) + self._test_retry(req, DNSLookupError('foo'), meta_max_retry_times) + + def test_without_metakey(self): + + # SETTINGS: RETRY_TIMES is NON-ZERO + self.mw.max_retry_times = 5 + + req = Request(self.invalid_url) + self._test_retry(req, DNSLookupError('foo'), self.mw.max_retry_times) + + def test_with_metakey_greater(self): + + # SETINGS: RETRY_TIMES < meta(max_retry_times) + self.mw.max_retry_times = 2 + meta_max_retry_times = 3 + + req1 = Request(self.invalid_url, meta={'max_retry_times': meta_max_retry_times}) + req2 = Request(self.invalid_url) + + self._test_retry(req1, DNSLookupError('foo'), meta_max_retry_times) + self._test_retry(req2, DNSLookupError('foo'), self.mw.max_retry_times) + + def test_with_metakey_lesser(self): + + # SETINGS: RETRY_TIMES > meta(max_retry_times) + self.mw.max_retry_times = 5 + meta_max_retry_times = 4 + + req1 = Request(self.invalid_url, meta={'max_retry_times': meta_max_retry_times}) + req2 = Request(self.invalid_url) + + self._test_retry(req1, DNSLookupError('foo'), meta_max_retry_times) + self._test_retry(req2, DNSLookupError('foo'), self.mw.max_retry_times) + + def test_with_dont_retry(self): + + # SETTINGS: meta(max_retry_times) = 4 + meta_max_retry_times = 4 + + req = Request(self.invalid_url, meta= \ + {'max_retry_times': meta_max_retry_times, 'dont_retry': True}) + self._test_retry(req, DNSLookupError('foo'), 0) def _test_retry(self, req, exception, max_retry_times): - while max_retry_times > 0: + for i in range(0, max_retry_times): req = self.mw.process_exception(req, exception, self.spider) assert isinstance(req, Request) - if req.meta['retry_times'] == max_retry_times: - break # discard it req = self.mw.process_exception(req, exception, self.spider)