From 6ce1ad31071326d22387f8444abefd6f3e18ed86 Mon Sep 17 00:00:00 2001 From: Eugenio Lacuesta Date: Fri, 10 Jan 2020 04:20:37 -0300 Subject: [PATCH 1/6] [test] Spider middleware: catch exceptions right after the spider callback --- tests/test_spidermiddleware_output_chain.py | 50 +++++++++++++++++++-- 1 file changed, 46 insertions(+), 4 deletions(-) diff --git a/tests/test_spidermiddleware_output_chain.py b/tests/test_spidermiddleware_output_chain.py index 739cf1c2d..b19a74609 100644 --- a/tests/test_spidermiddleware_output_chain.py +++ b/tests/test_spidermiddleware_output_chain.py @@ -1,10 +1,10 @@ - from testfixtures import LogCapture -from twisted.trial.unittest import TestCase from twisted.internet import defer +from twisted.trial.unittest import TestCase -from scrapy import Spider, Request +from scrapy import Request, Spider from scrapy.utils.test import get_crawler + from tests.mockserver import MockServer @@ -74,7 +74,7 @@ class ProcessSpiderInputSpiderWithErrback(ProcessSpiderInputSpiderWithoutErrback name = 'ProcessSpiderInputSpiderWithErrback' def start_requests(self): - yield Request(url=self.mockserver.url('/status?n=200'), callback=self.parse, errback=self.errback) + yield Request(self.mockserver.url('/status?n=200'), self.parse, errback=self.errback) def errback(self, failure): self.logger.info('Got a Failure on the Request errback') @@ -100,6 +100,17 @@ class GeneratorCallbackSpider(Spider): raise ImportError() +# ================================================================================ +# (2.1) exceptions from a spider callback (generator, middleware right after callback) +class GeneratorCallbackSpiderMiddlewareRightAfterSpider(GeneratorCallbackSpider): + name = 'GeneratorCallbackSpiderMiddlewareRightAfterSpider' + custom_settings = { + 'SPIDER_MIDDLEWARES': { + __name__ + '.LogExceptionMiddleware': 100000, + }, + } + + # ================================================================================ # (3) exceptions from a spider callback (not a generator) class NotGeneratorCallbackSpider(Spider): @@ -117,6 +128,17 @@ class NotGeneratorCallbackSpider(Spider): return [{'test': 1}, {'test': 1/0}] +# ================================================================================ +# (3.1) exceptions from a spider callback (not a generator, middleware right after callback) +class NotGeneratorCallbackSpiderMiddlewareRightAfterSpider(NotGeneratorCallbackSpider): + name = 'NotGeneratorCallbackSpiderMiddlewareRightAfterSpider' + custom_settings = { + 'SPIDER_MIDDLEWARES': { + __name__ + '.LogExceptionMiddleware': 100000, + }, + } + + # ================================================================================ # (4) exceptions from a middleware process_spider_output method (generator) class GeneratorOutputChainSpider(Spider): @@ -320,6 +342,16 @@ class TestSpiderMiddleware(TestCase): self.assertIn("Middleware: ImportError exception caught", str(log2)) self.assertIn("'item_scraped_count': 2", str(log2)) + @defer.inlineCallbacks + def test_generator_callback_right_after_callback(self): + """ + (2.1) Special case of (2): Exceptions should be caught + even if the middleware is placed right after the spider + """ + log21 = yield self.crawl_log(GeneratorCallbackSpiderMiddlewareRightAfterSpider) + self.assertIn("Middleware: ImportError exception caught", str(log21)) + self.assertIn("'item_scraped_count': 2", str(log21)) + @defer.inlineCallbacks def test_not_a_generator_callback(self): """ @@ -330,6 +362,16 @@ class TestSpiderMiddleware(TestCase): self.assertIn("Middleware: ZeroDivisionError exception caught", str(log3)) self.assertNotIn("item_scraped_count", str(log3)) + @defer.inlineCallbacks + def test_not_a_generator_callback_right_after_callback(self): + """ + (3.1) Special case of (3): Exceptions should be caught + even if the middleware is placed right after the spider + """ + log31 = yield self.crawl_log(NotGeneratorCallbackSpiderMiddlewareRightAfterSpider) + self.assertIn("Middleware: ZeroDivisionError exception caught", str(log31)) + self.assertNotIn("item_scraped_count", str(log31)) + @defer.inlineCallbacks def test_generator_output_chain(self): """ From c088c04f449c3383b7867c25b387f13949b1d6c0 Mon Sep 17 00:00:00 2001 From: Eugenio Lacuesta Date: Fri, 10 Jan 2020 04:20:55 -0300 Subject: [PATCH 2/6] Spider middleware: catch exceptions right after the spider callback --- scrapy/core/spidermw.py | 71 +++++++++++++++++++++++++---------------- scrapy/utils/python.py | 1 + 2 files changed, 44 insertions(+), 28 deletions(-) diff --git a/scrapy/core/spidermw.py b/scrapy/core/spidermw.py index 097a374bf..180a0b1fe 100644 --- a/scrapy/core/spidermw.py +++ b/scrapy/core/spidermw.py @@ -3,13 +3,14 @@ Spider Middleware manager See documentation in docs/topics/spider-middleware.rst """ -from itertools import chain, islice +from itertools import islice from twisted.python.failure import Failure + from scrapy.exceptions import _InvalidOutput from scrapy.middleware import MiddlewareManager -from scrapy.utils.defer import mustbe_deferred from scrapy.utils.conf import build_component_list +from scrapy.utils.defer import mustbe_deferred from scrapy.utils.python import MutableChain @@ -17,6 +18,13 @@ def _isiterable(possible_iterator): return hasattr(possible_iterator, '__iter__') +def _fname(f): + return "%s.%s".format( + f.__self__.__class__.__name__, + f.__func__.__name__ + ) + + class SpiderMiddlewareManager(MiddlewareManager): component_name = 'spider middleware' @@ -31,27 +39,36 @@ class SpiderMiddlewareManager(MiddlewareManager): self.methods['process_spider_input'].append(mw.process_spider_input) if hasattr(mw, 'process_start_requests'): self.methods['process_start_requests'].appendleft(mw.process_start_requests) - self.methods['process_spider_output'].appendleft(getattr(mw, 'process_spider_output', None)) - self.methods['process_spider_exception'].appendleft(getattr(mw, 'process_spider_exception', None)) + process_spider_output = getattr(mw, 'process_spider_output', None) + self.methods['process_spider_output'].appendleft(process_spider_output) + process_spider_exception = getattr(mw, 'process_spider_exception', None) + self.methods['process_spider_exception'].appendleft(process_spider_exception) def scrape_response(self, scrape_func, response, request, spider): - fname = lambda f: '%s.%s' % ( - f.__self__.__class__.__name__, - f.__func__.__name__) def process_spider_input(response): for method in self.methods['process_spider_input']: try: result = method(response=response, spider=spider) if result is not None: - raise _InvalidOutput('Middleware {} must return None or raise an exception, got {}' - .format(fname(method), type(result))) + msg = "Middleware {} must return None or raise an exception, got {}" + raise _InvalidOutput(msg.format(_fname(method), type(result))) except _InvalidOutput: raise except Exception: return scrape_func(Failure(), request, spider) return scrape_func(response, request, spider) + def _evaluate_iterable(iterable, method_index, recover_to): + try: + for r in iterable: + yield r + except Exception as ex: + exception_result = process_spider_exception(Failure(ex), method_index) + if isinstance(exception_result, Failure): + raise + recover_to.extend(exception_result) + def process_spider_exception(_failure, start_index=0): exception = _failure.value # don't handle _InvalidOutput exception @@ -69,8 +86,8 @@ class SpiderMiddlewareManager(MiddlewareManager): elif result is None: continue else: - raise _InvalidOutput('Middleware {} must return None or an iterable, got {}' - .format(fname(method), type(result))) + msg = "Middleware {} must return None or an iterable, got {}" + raise _InvalidOutput(msg.format(_fname(method), type(result))) return _failure def process_spider_output(result, start_index=0): @@ -78,38 +95,36 @@ class SpiderMiddlewareManager(MiddlewareManager): # chain, they went through it already from the process_spider_exception method recovered = MutableChain() - def evaluate_iterable(iterable, index): - try: - for r in iterable: - yield r - except Exception as ex: - exception_result = process_spider_exception(Failure(ex), index+1) - if isinstance(exception_result, Failure): - raise - recovered.extend(exception_result) - method_list = islice(self.methods['process_spider_output'], start_index, None) for method_index, method in enumerate(method_list, start=start_index): if method is None: continue - # the following might fail directly if the output value is not a generator try: + # might fail directly if the output value is not a generator result = method(response=response, result=result, spider=spider) except Exception as ex: exception_result = process_spider_exception(Failure(ex), method_index+1) if isinstance(exception_result, Failure): raise return exception_result - if _isiterable(result): - result = evaluate_iterable(result, method_index) else: - raise _InvalidOutput('Middleware {} must return an iterable, got {}' - .format(fname(method), type(result))) + if _isiterable(result): + result = _evaluate_iterable(result, method_index+1, recovered) + else: + msg = "Middleware {} must return an iterable, got {}" + raise _InvalidOutput(msg.format(_fname(method), type(result))) - return chain(result, recovered) + return MutableChain(result, recovered) + + def process_callback_output(result): + if isinstance(result, Failure): + return process_spider_exception(result) + recovered = MutableChain() + result = _evaluate_iterable(result, 0, recovered) + return MutableChain(process_spider_output(result), recovered) dfd = mustbe_deferred(process_spider_input, response) - dfd.addCallbacks(callback=process_spider_output, errback=process_spider_exception) + dfd.addCallbacks(callback=process_callback_output, errback=process_callback_output) return dfd def process_start_requests(self, start_requests, spider): diff --git a/scrapy/utils/python.py b/scrapy/utils/python.py index 8d829c5a5..875650f3e 100644 --- a/scrapy/utils/python.py +++ b/scrapy/utils/python.py @@ -375,6 +375,7 @@ class MutableChain(object): """ Thin wrapper around itertools.chain, allowing to add iterables "in-place" """ + def __init__(self, *args): self.data = chain(*args) From d6e928f47209396d0f6c0b155eb07115c4855c93 Mon Sep 17 00:00:00 2001 From: Eugenio Lacuesta Date: Fri, 10 Jan 2020 04:40:03 -0300 Subject: [PATCH 3/6] Remove object as base class for MutableChain Plus some minor styling adjustments --- scrapy/utils/python.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scrapy/utils/python.py b/scrapy/utils/python.py index 875650f3e..e5582cc18 100644 --- a/scrapy/utils/python.py +++ b/scrapy/utils/python.py @@ -1,15 +1,15 @@ """ This module contains essential stuff that should've come with Python itself ;) """ +import errno import gc +import inspect import os import re -import inspect +import sys import weakref -import errno from functools import partial, wraps from itertools import chain -import sys from scrapy.utils.decorators import deprecated @@ -371,7 +371,7 @@ else: gc.collect() -class MutableChain(object): +class MutableChain: """ Thin wrapper around itertools.chain, allowing to add iterables "in-place" """ From 9770ca35fb494503298270902569ed897a365d32 Mon Sep 17 00:00:00 2001 From: Eugenio Lacuesta Date: Fri, 10 Jan 2020 18:45:39 -0300 Subject: [PATCH 4/6] Spider middleware: simplify deferred errback handling --- scrapy/core/spidermw.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/scrapy/core/spidermw.py b/scrapy/core/spidermw.py index 180a0b1fe..ed02b306b 100644 --- a/scrapy/core/spidermw.py +++ b/scrapy/core/spidermw.py @@ -117,14 +117,12 @@ class SpiderMiddlewareManager(MiddlewareManager): return MutableChain(result, recovered) def process_callback_output(result): - if isinstance(result, Failure): - return process_spider_exception(result) recovered = MutableChain() result = _evaluate_iterable(result, 0, recovered) return MutableChain(process_spider_output(result), recovered) dfd = mustbe_deferred(process_spider_input, response) - dfd.addCallbacks(callback=process_callback_output, errback=process_callback_output) + dfd.addCallbacks(callback=process_callback_output, errback=process_spider_exception) return dfd def process_start_requests(self, start_requests, spider): From ad4477d335bee8b10bc3bbca969defddd9b316f8 Mon Sep 17 00:00:00 2001 From: Eugenio Lacuesta Date: Mon, 27 Jan 2020 14:16:43 -0300 Subject: [PATCH 5/6] Remove unnecessary else --- scrapy/core/spidermw.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/scrapy/core/spidermw.py b/scrapy/core/spidermw.py index ed02b306b..8b36cbb04 100644 --- a/scrapy/core/spidermw.py +++ b/scrapy/core/spidermw.py @@ -107,12 +107,11 @@ class SpiderMiddlewareManager(MiddlewareManager): if isinstance(exception_result, Failure): raise return exception_result + if _isiterable(result): + result = _evaluate_iterable(result, method_index+1, recovered) else: - if _isiterable(result): - result = _evaluate_iterable(result, method_index+1, recovered) - else: - msg = "Middleware {} must return an iterable, got {}" - raise _InvalidOutput(msg.format(_fname(method), type(result))) + msg = "Middleware {} must return an iterable, got {}" + raise _InvalidOutput(msg.format(_fname(method), type(result))) return MutableChain(result, recovered) From fbea370c58c1d82b52fd9c1f7d3a6cee94477c7a Mon Sep 17 00:00:00 2001 From: Eugenio Lacuesta Date: Wed, 5 Feb 2020 01:35:13 -0300 Subject: [PATCH 6/6] Rename function parameter --- scrapy/core/spidermw.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scrapy/core/spidermw.py b/scrapy/core/spidermw.py index 8b36cbb04..dd9b3c376 100644 --- a/scrapy/core/spidermw.py +++ b/scrapy/core/spidermw.py @@ -59,12 +59,12 @@ class SpiderMiddlewareManager(MiddlewareManager): return scrape_func(Failure(), request, spider) return scrape_func(response, request, spider) - def _evaluate_iterable(iterable, method_index, recover_to): + def _evaluate_iterable(iterable, exception_processor_index, recover_to): try: for r in iterable: yield r except Exception as ex: - exception_result = process_spider_exception(Failure(ex), method_index) + exception_result = process_spider_exception(Failure(ex), exception_processor_index) if isinstance(exception_result, Failure): raise recover_to.extend(exception_result)