From e285b1d6c2aaa1fdfe788f1894b0196bc64d1be1 Mon Sep 17 00:00:00 2001 From: Mikhail Korobov Date: Tue, 7 Feb 2017 18:17:07 +0500 Subject: [PATCH 1/3] retry stats --- scrapy/downloadermiddlewares/retry.py | 14 ++++++++++++-- scrapy/downloadermiddlewares/stats.py | 4 +++- scrapy/utils/misc.py | 2 +- scrapy/utils/python.py | 11 +++++++++++ scrapy/utils/response.py | 3 ++- tests/test_downloadermiddleware_retry.py | 15 ++++++++++++--- tests/test_proxy_connect.py | 4 +++- 7 files changed, 44 insertions(+), 9 deletions(-) diff --git a/scrapy/downloadermiddlewares/retry.py b/scrapy/downloadermiddlewares/retry.py index c9c512be8..d84697b14 100644 --- a/scrapy/downloadermiddlewares/retry.py +++ b/scrapy/downloadermiddlewares/retry.py @@ -22,6 +22,7 @@ from twisted.web.client import ResponseFailed from scrapy.exceptions import NotConfigured from scrapy.utils.response import response_status_message from scrapy.core.downloader.handlers.http11 import TunnelError +from scrapy.utils.python import global_object_name logger = logging.getLogger(__name__) @@ -35,16 +36,18 @@ class RetryMiddleware(object): ConnectionLost, TCPTimedOutError, ResponseFailed, IOError, TunnelError) - def __init__(self, settings): + def __init__(self, crawler): + settings = crawler.settings if not settings.getbool('RETRY_ENABLED'): raise NotConfigured self.max_retry_times = settings.getint('RETRY_TIMES') self.retry_http_codes = set(int(x) for x in settings.getlist('RETRY_HTTP_CODES')) self.priority_adjust = settings.getint('RETRY_PRIORITY_ADJUST') + self.stats = crawler.stats @classmethod def from_crawler(cls, crawler): - return cls(crawler.settings) + return cls(crawler) def process_response(self, request, response, spider): if request.meta.get('dont_retry', False): @@ -70,8 +73,15 @@ class RetryMiddleware(object): retryreq.meta['retry_times'] = retries retryreq.dont_filter = True retryreq.priority = request.priority + self.priority_adjust + + if isinstance(reason, Exception): + reason = global_object_name(reason.__class__) + + self.stats.inc_value('retry/count') + self.stats.inc_value('retry/reason_count/%s' % reason) return retryreq else: + self.stats.inc_value('retry/max_reached') logger.debug("Gave up retrying %(request)s (failed %(retries)d times): %(reason)s", {'request': request, 'retries': retries, 'reason': reason}, extra={'spider': spider}) diff --git a/scrapy/downloadermiddlewares/stats.py b/scrapy/downloadermiddlewares/stats.py index 9c0ad90a5..ef0aafce0 100644 --- a/scrapy/downloadermiddlewares/stats.py +++ b/scrapy/downloadermiddlewares/stats.py @@ -1,6 +1,8 @@ from scrapy.exceptions import NotConfigured from scrapy.utils.request import request_httprepr from scrapy.utils.response import response_httprepr +from scrapy.utils.python import global_object_name + class DownloaderStats(object): @@ -27,6 +29,6 @@ class DownloaderStats(object): return response def process_exception(self, request, exception, spider): - ex_class = "%s.%s" % (exception.__class__.__module__, exception.__class__.__name__) + ex_class = global_object_name(exception.__class__) self.stats.inc_value('downloader/exception_count', spider=spider) self.stats.inc_value('downloader/exception_type_count/%s' % ex_class, spider=spider) diff --git a/scrapy/utils/misc.py b/scrapy/utils/misc.py index 30c9e5058..35f855007 100644 --- a/scrapy/utils/misc.py +++ b/scrapy/utils/misc.py @@ -113,7 +113,7 @@ def md5sum(file): m.update(d) return m.hexdigest() + def rel_has_nofollow(rel): """Return True if link rel attribute has nofollow type""" return True if rel is not None and 'nofollow' in rel.split() else False - diff --git a/scrapy/utils/python.py b/scrapy/utils/python.py index 42fbbda7f..4c500abf4 100644 --- a/scrapy/utils/python.py +++ b/scrapy/utils/python.py @@ -344,3 +344,14 @@ def without_none_values(iterable): return {k: v for k, v in six.iteritems(iterable) if v is not None} except AttributeError: return type(iterable)((v for v in iterable if v is not None)) + + +def global_object_name(obj): + """ + Return full name of a global object. + + >>> from scrapy import Request + >>> global_object_name(Request) + 'scrapy.http.request.Request' + """ + return "%s.%s" % (obj.__module__, obj.__name__) diff --git a/scrapy/utils/response.py b/scrapy/utils/response.py index deb5741be..bf276b5ca 100644 --- a/scrapy/utils/response.py +++ b/scrapy/utils/response.py @@ -43,7 +43,8 @@ def get_meta_refresh(response): def response_status_message(status): """Return status code plus status text descriptive message """ - return '%s %s' % (status, to_native_str(http.RESPONSES.get(int(status), "Unknown Status"))) + message = http.RESPONSES.get(int(status), "Unknown Status") + return '%s %s' % (status, to_native_str(message)) def response_httprepr(response): diff --git a/tests/test_downloadermiddleware_retry.py b/tests/test_downloadermiddleware_retry.py index e129b71f8..b833cb448 100644 --- a/tests/test_downloadermiddleware_retry.py +++ b/tests/test_downloadermiddleware_retry.py @@ -13,9 +13,9 @@ from scrapy.utils.test import get_crawler class RetryTest(unittest.TestCase): def setUp(self): - crawler = get_crawler(Spider) - self.spider = crawler._create_spider('foo') - self.mw = RetryMiddleware.from_crawler(crawler) + 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_priority_adjust(self): @@ -70,6 +70,10 @@ class RetryTest(unittest.TestCase): # discard it assert self.mw.process_response(req, rsp, self.spider) is rsp + assert self.crawler.stats.get_value('retry/max_reached') == 1 + assert self.crawler.stats.get_value('retry/reason_count/503 Service Unavailable') == 2 + assert self.crawler.stats.get_value('retry/count') == 2 + def test_twistederrors(self): exceptions = [defer.TimeoutError, TCPTimedOutError, TimeoutError, DNSLookupError, ConnectionRefusedError, ConnectionDone, @@ -79,6 +83,11 @@ class RetryTest(unittest.TestCase): req = Request('http://www.scrapytest.org/%s' % exc.__name__) self._test_retry_exception(req, exc('foo')) + stats = self.crawler.stats + assert stats.get_value('retry/max_reached') == len(exceptions) + assert stats.get_value('retry/count') == len(exceptions) * 2 + assert stats.get_value('retry/reason_count/twisted.internet.defer.TimeoutError') == 2 + def _test_retry_exception(self, req, exception): # first retry req = self.mw.process_exception(req, exception, self.spider) diff --git a/tests/test_proxy_connect.py b/tests/test_proxy_connect.py index 0f06fd53d..6213a51e8 100644 --- a/tests/test_proxy_connect.py +++ b/tests/test_proxy_connect.py @@ -101,7 +101,9 @@ class ProxyConnectTestCase(TestCase): self._assert_got_response_code(407, l) def _assert_got_response_code(self, code, log): + print(log) self.assertEqual(str(log).count('Crawled (%d)' % code), 1) def _assert_got_tunnel_error(self, log): - self.assertEqual(str(log).count('TunnelError'), 1) + print(log) + self.assertIn('TunnelError', str(log)) From 39df675f091cf904dc904acd9538a0bebe2e55cd Mon Sep 17 00:00:00 2001 From: Mikhail Korobov Date: Tue, 14 Feb 2017 23:28:50 +0500 Subject: [PATCH 2/3] make retry middleware changes backwards compatible --- scrapy/downloadermiddlewares/retry.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/scrapy/downloadermiddlewares/retry.py b/scrapy/downloadermiddlewares/retry.py index d84697b14..549d74f46 100644 --- a/scrapy/downloadermiddlewares/retry.py +++ b/scrapy/downloadermiddlewares/retry.py @@ -36,18 +36,16 @@ class RetryMiddleware(object): ConnectionLost, TCPTimedOutError, ResponseFailed, IOError, TunnelError) - def __init__(self, crawler): - settings = crawler.settings + def __init__(self, settings): if not settings.getbool('RETRY_ENABLED'): raise NotConfigured self.max_retry_times = settings.getint('RETRY_TIMES') self.retry_http_codes = set(int(x) for x in settings.getlist('RETRY_HTTP_CODES')) self.priority_adjust = settings.getint('RETRY_PRIORITY_ADJUST') - self.stats = crawler.stats @classmethod def from_crawler(cls, crawler): - return cls(crawler) + return cls(crawler.settings) def process_response(self, request, response, spider): if request.meta.get('dont_retry', False): @@ -65,6 +63,7 @@ class RetryMiddleware(object): def _retry(self, request, reason, spider): retries = request.meta.get('retry_times', 0) + 1 + stats = spider.crawler.stats if retries <= self.max_retry_times: logger.debug("Retrying %(request)s (failed %(retries)d times): %(reason)s", {'request': request, 'retries': retries, 'reason': reason}, @@ -77,11 +76,11 @@ class RetryMiddleware(object): if isinstance(reason, Exception): reason = global_object_name(reason.__class__) - self.stats.inc_value('retry/count') - self.stats.inc_value('retry/reason_count/%s' % reason) + stats.inc_value('retry/count') + stats.inc_value('retry/reason_count/%s' % reason) return retryreq else: - self.stats.inc_value('retry/max_reached') + stats.inc_value('retry/max_reached') logger.debug("Gave up retrying %(request)s (failed %(retries)d times): %(reason)s", {'request': request, 'retries': retries, 'reason': reason}, extra={'spider': spider}) From 0b90c3b43c4eedc891056fa433e0608afbe6cd32 Mon Sep 17 00:00:00 2001 From: Paul Tremberth Date: Mon, 27 Feb 2017 17:42:00 +0100 Subject: [PATCH 3/3] Re-enable FTP tests on Python 3 --- scrapy/core/downloader/handlers/ftp.py | 7 ++++--- tests/py3-ignores.txt | 7 ------- tests/test_downloader_handlers.py | 27 ++++++++++++-------------- 3 files changed, 16 insertions(+), 25 deletions(-) diff --git a/scrapy/core/downloader/handlers/ftp.py b/scrapy/core/downloader/handlers/ftp.py index 1398140b4..933bc7e8d 100644 --- a/scrapy/core/downloader/handlers/ftp.py +++ b/scrapy/core/downloader/handlers/ftp.py @@ -39,12 +39,13 @@ from twisted.internet.protocol import Protocol, ClientCreator from scrapy.http import Response from scrapy.responsetypes import responsetypes from scrapy.utils.httpobj import urlparse_cached +from scrapy.utils.python import to_bytes class ReceivedDataProtocol(Protocol): def __init__(self, filename=None): self.__filename = filename - self.body = open(filename, "w") if filename else BytesIO() + self.body = open(filename, "wb") if filename else BytesIO() self.size = 0 def dataReceived(self, data): @@ -97,7 +98,7 @@ class FTPDownloadHandler(object): protocol.close() body = protocol.filename or protocol.body.read() headers = {"local filename": protocol.filename or '', "size": protocol.size} - return respcls(url=request.url, status=200, body=body, headers=headers) + return respcls(url=request.url, status=200, body=to_bytes(body), headers=headers) def _failed(self, result, request): message = result.getErrorMessage() @@ -106,6 +107,6 @@ class FTPDownloadHandler(object): if m: ftpcode = m.group() httpcode = self.CODE_MAPPING.get(ftpcode, self.CODE_MAPPING["default"]) - return Response(url=request.url, status=httpcode, body=message) + return Response(url=request.url, status=httpcode, body=to_bytes(message)) raise result.type(result.value) diff --git a/tests/py3-ignores.txt b/tests/py3-ignores.txt index ec2947003..313e74ec9 100644 --- a/tests/py3-ignores.txt +++ b/tests/py3-ignores.txt @@ -1,13 +1,6 @@ tests/test_linkextractors_deprecated.py tests/test_proxy_connect.py -scrapy/xlib/tx/iweb.py -scrapy/xlib/tx/interfaces.py -scrapy/xlib/tx/endpoints.py -scrapy/xlib/tx/client.py -scrapy/xlib/tx/_newclient.py -scrapy/xlib/tx/__init__.py -scrapy/core/downloader/handlers/ftp.py scrapy/linkextractors/sgml.py scrapy/linkextractors/regex.py scrapy/linkextractors/htmlparser.py diff --git a/tests/test_downloader_handlers.py b/tests/test_downloader_handlers.py index c1683fb3e..e49a514b8 100644 --- a/tests/test_downloader_handlers.py +++ b/tests/test_downloader_handlers.py @@ -687,9 +687,6 @@ class BaseFTPTestCase(unittest.TestCase): password = "passwd" req_meta = {"ftp_user": username, "ftp_password": password} - if six.PY3: - skip = "Twisted missing ftp support for PY3" - def setUp(self): from twisted.protocols.ftp import FTPRealm, FTPFactory from scrapy.core.downloader.handlers.ftp import FTPDownloadHandler @@ -700,8 +697,8 @@ class BaseFTPTestCase(unittest.TestCase): userdir = os.path.join(self.directory, self.username) os.mkdir(userdir) fp = FilePath(userdir) - fp.child('file.txt').setContent("I have the power!") - fp.child('file with spaces.txt').setContent("Moooooooooo power!") + fp.child('file.txt').setContent(b"I have the power!") + fp.child('file with spaces.txt').setContent(b"Moooooooooo power!") # setup server realm = FTPRealm(anonymousRoot=self.directory, userHome=self.directory) @@ -736,8 +733,8 @@ class BaseFTPTestCase(unittest.TestCase): def _test(r): self.assertEqual(r.status, 200) - self.assertEqual(r.body, 'I have the power!') - self.assertEqual(r.headers, {'Local Filename': [''], 'Size': ['17']}) + self.assertEqual(r.body, b'I have the power!') + self.assertEqual(r.headers, {b'Local Filename': [b''], b'Size': [b'17']}) return self._add_test_callbacks(d, _test) def test_ftp_download_path_with_spaces(self): @@ -749,8 +746,8 @@ class BaseFTPTestCase(unittest.TestCase): def _test(r): self.assertEqual(r.status, 200) - self.assertEqual(r.body, 'Moooooooooo power!') - self.assertEqual(r.headers, {'Local Filename': [''], 'Size': ['18']}) + self.assertEqual(r.body, b'Moooooooooo power!') + self.assertEqual(r.headers, {b'Local Filename': [b''], b'Size': [b'18']}) return self._add_test_callbacks(d, _test) def test_ftp_download_notexist(self): @@ -763,7 +760,7 @@ class BaseFTPTestCase(unittest.TestCase): return self._add_test_callbacks(d, _test) def test_ftp_local_filename(self): - local_fname = "/tmp/file.txt" + local_fname = b"/tmp/file.txt" meta = {"ftp_local_filename": local_fname} meta.update(self.req_meta) request = Request(url="ftp://127.0.0.1:%s/file.txt" % self.portNum, @@ -772,10 +769,10 @@ class BaseFTPTestCase(unittest.TestCase): def _test(r): self.assertEqual(r.body, local_fname) - self.assertEqual(r.headers, {'Local Filename': ['/tmp/file.txt'], 'Size': ['17']}) + self.assertEqual(r.headers, {b'Local Filename': [b'/tmp/file.txt'], b'Size': [b'17']}) self.assertTrue(os.path.exists(local_fname)) - with open(local_fname) as f: - self.assertEqual(f.read(), "I have the power!") + with open(local_fname, "rb") as f: + self.assertEqual(f.read(), b"I have the power!") os.remove(local_fname) return self._add_test_callbacks(d, _test) @@ -810,8 +807,8 @@ class AnonymousFTPTestCase(BaseFTPTestCase): os.mkdir(self.directory) fp = FilePath(self.directory) - fp.child('file.txt').setContent("I have the power!") - fp.child('file with spaces.txt').setContent("Moooooooooo power!") + fp.child('file.txt').setContent(b"I have the power!") + fp.child('file with spaces.txt').setContent(b"Moooooooooo power!") # setup server for anonymous access realm = FTPRealm(anonymousRoot=self.directory)