Repository: scrapy/scrapy
Pull request: [MRG+1] process_spider_exception on generators
This PR is a starting point to fix #220.
It could probably use some more test cases, mostly to figure out what exactly is the desired behaviour when processing the exceptions.
I can't take much credit for this: if it breaks, blame @dangra 😛 


Problem statement:
process_spider_exception() not invoked for generators
When a spider implements _parse()_ function through generator (i.e. returning results with _yields_) and an exception raises in _parse()_, then spidermiddleware's _process_spider_exception()_ is not called.

The reason is that exception is finally invoked in one of the spidermiddlewares.

Hints from issue discussion:
bug for sure, do you have an idea of what can be done to fix it? 

A question for the beginning. What is the desired behavior?:
1. when the exception in spider raises, all the previously yielded results get lost and only the exception passes through spidermiddleware's process_spider_exception()
2. results yielded before the exception was raised are processed normally, i.e. create some kind of sublist of them and pass it through spidermiddleware's process_spider_output(). Then raise the exception and pass it through spidermiddleware's process_spider_exception().

On one hand, it seems more intuitive to process the results yielded before the error normally. On the other hand, invoking process_spider_output() and process_spider_exception() for the same request somehow doesn't feel right.

In either solution it seems to me, that it can't be done without collecting all the results from the generator, first, to invoke the exception, if there is one. Which kind of sucks, if the intention to use generator is the fact that there are too many results to fit in the memory or something like that.

More like option 2 but do not collect all generator results into a list.

There are two kind of spider errors, one that fails before first result is yielded (this is what Scrapy catches now), and another that raises after generator yielded some results (the one you pointed)

For the second case, I think we can call `process_spider_output` til the generator fails and then call `process_spider_exception` even if called for same request-response pair.

``` python
def spidercallback(maxn, failat, failfast):
    if failfast:
        1 / 0 

    for n in xrange(maxn):
        if n == failat:
            1 / 0 
        yield n

def errtrap(testid, exc):
    print testid, 'ERROR', exc 

def scrapercall(testid, maxn=10, failat=2, failfast=False):
    try:
        for n in spidercallback(maxn, failat, failfast):
            print testid, 'OK', n
    except Exception as exc:
        errtrap(testid, exc)

scrapercall(1, failat=2)
scrapercall(2, failfast=True)
```

outputs:

```
1 OK 0
1 OK 1
1 ERROR integer division or modulo by zero
2 ERROR integer division or modulo by zero
```

Has there been any progress with this bug? It's still present in the current version of Scrapy.

it's still present

Hi! I want to start contributing to scrapy fixing this issue. I want to fix the bug using the solution proposed by @dangra: call `process_spider_output` til the generator fails and then call spidermiddleware's `process_spider_exception`. 

I have been looking scrapy source code for a while now and I think the solution must be somewhere in here [1](https://github.com/scrapy/scrapy/blob/master/scrapy/core/spidermw.py#L69), [2](https://github.com/scrapy/scrapy/blob/master/scrapy/core/scraper.py#L127) but still can't figure out where should the fix go. I also already looked at twisted Deferreds documentation, so I know more or less how it works :)

From what I have understood, the spidermiddleware `process_spider_output` and `process_spider_exception` are called [here](https://github.com/scrapy/scrapy/blob/master/scrapy/core/spidermw.py#L69):

```
dfd = mustbe_deferred(process_spider_input, response)
dfd.addErrback(process_spider_exception)
dfd.addCallback(process_spider_output)
```

But the spider `parse()` returns a generator right? so `process_spider_output` is called because no exception has been raise yet. Because `process_spider_output` is called, the scraper thinks that everything is fine, so each spidermiddleware `process_spider_output` is called, where finally the exception is invoked. 

I think that I have a good understanding of what's happening below the hood, but I still think that I'm missing something, that's why I wanted to ask if someone can guide me a little here. 

Thanks!!

I would like to work on this issue.I would like to know how should I start with it?

https://github.com/llonchj/scrapy-sentry

That module will help
To whom it may concern, I used **spider_error** signal as a work-around for this issue (https://doc.scrapy.org/en/latest/topics/signals.html)
i was trying to make a custom spider middleware to handle exceptions with process_spider_exception method but still doesn't work

@jeebb that saved me, thanks!

Commit message: Force Travis build

Patch to review:
diff --git a/docs/topics/spider-middleware.rst b/docs/topics/spider-middleware.rst
index 2b7e427714f..7db623cf49f 100644
--- a/docs/topics/spider-middleware.rst
+++ b/docs/topics/spider-middleware.rst
@@ -78,7 +78,8 @@ following methods:
 
         If it raises an exception, Scrapy won't bother calling any other spider
         middleware :meth:`process_spider_input` and will call the request
-        errback.  The output of the errback is chained back in the other
+        errback if there is one, otherwise it will start the :meth:`process_spider_exception`
+        chain. The output of the errback is chained back in the other
         direction for :meth:`process_spider_output` to process it, or
         :meth:`process_spider_exception` if it raised an exception.
 
@@ -112,8 +113,8 @@ following methods:
 
     .. method:: process_spider_exception(response, exception, spider)
 
-        This method is called when a spider or :meth:`process_spider_input`
-        method (from other spider middleware) raises an exception.
+        This method is called when a spider or :meth:`process_spider_output`
+        method (from a previous spider middleware) raises an exception.
 
         :meth:`process_spider_exception` should return either ``None`` or an
         iterable of :class:`~scrapy.http.Request`, dict or
@@ -125,7 +126,8 @@ following methods:
         exception reaches the engine (where it's logged and discarded).
 
         If it returns an iterable the :meth:`process_spider_output` pipeline
-        kicks in, and no other :meth:`process_spider_exception` will be called.
+        kicks in, starting from the next spider middleware, and no other
+        :meth:`process_spider_exception` will be called.
 
         :param response: the response being processed when the exception was
           raised
diff --git a/scrapy/core/downloader/middleware.py b/scrapy/core/downloader/middleware.py
index c3b23e28490..2fa277e7da6 100644
--- a/scrapy/core/downloader/middleware.py
+++ b/scrapy/core/downloader/middleware.py
@@ -7,6 +7,7 @@
 
 from twisted.internet import defer
 
+from scrapy.exceptions import _InvalidOutput
 from scrapy.http import Request, Response
 from scrapy.middleware import MiddlewareManager
 from scrapy.utils.defer import mustbe_deferred
@@ -35,9 +36,9 @@ def download(self, download_func, request, spider):
         def process_request(request):
             for method in self.methods['process_request']:
                 response = yield method(request=request, spider=spider)
-                assert response is None or isinstance(response, (Response, Request)), \
-                        'Middleware %s.process_request must return None, Response or Request, got %s' % \
-                        (six.get_method_self(method).__class__.__name__, response.__class__.__name__)
+                if response is not None and not isinstance(response, (Response, Request)):
+                    raise _InvalidOutput('Middleware %s.process_request must return None, Response or Request, got %s' % \
+                                         (six.get_method_self(method).__class__.__name__, response.__class__.__name__))
                 if response:
                     defer.returnValue(response)
             defer.returnValue((yield download_func(request=request,spider=spider)))
@@ -49,11 +50,10 @@ def process_response(response):
                 defer.returnValue(response)
 
             for method in self.methods['process_response']:
-                response = yield method(request=request, response=response,
-                                        spider=spider)
-                assert isinstance(response, (Response, Request)), \
-                    'Middleware %s.process_response must return Response or Request, got %s' % \
-                    (six.get_method_self(method).__class__.__name__, type(response))
+                response = yield method(request=request, response=response, spider=spider)
+                if not isinstance(response, (Response, Request)):
+                    raise _InvalidOutput('Middleware %s.process_response must return Response or Request, got %s' % \
+                                         (six.get_method_self(method).__class__.__name__, type(response)))
                 if isinstance(response, Request):
                     defer.returnValue(response)
             defer.returnValue(response)
@@ -62,11 +62,10 @@ def process_response(response):
         def process_exception(_failure):
             exception = _failure.value
             for method in self.methods['process_exception']:
-                response = yield method(request=request, exception=exception,
-                                        spider=spider)
-                assert response is None or isinstance(response, (Response, Request)), \
-                    'Middleware %s.process_exception must return None, Response or Request, got %s' % \
-                    (six.get_method_self(method).__class__.__name__, type(response))
+                response = yield method(request=request, exception=exception, spider=spider)
+                if response is not None and not isinstance(response, (Response, Request)):
+                    raise _InvalidOutput('Middleware %s.process_exception must return None, Response or Request, got %s' % \
+                                         (six.get_method_self(method).__class__.__name__, type(response)))
                 if response:
                     defer.returnValue(response)
             defer.returnValue(_failure)
diff --git a/scrapy/core/scraper.py b/scrapy/core/scraper.py
index ee1e95a0cd4..d7fe721fb29 100644
--- a/scrapy/core/scraper.py
+++ b/scrapy/core/scraper.py
@@ -135,7 +135,6 @@ def _scrape2(self, request_result, request, spider):
             return self.spidermw.scrape_response(
                 self.call_spider, request_result, request, spider)
         else:
-            # FIXME: don't ignore errors in spider middleware
             dfd = self.call_spider(request_result, request, spider)
             return dfd.addErrback(
                 self._log_download_errors, request_result, request, spider)
diff --git a/scrapy/core/spidermw.py b/scrapy/core/spidermw.py
index a206e4b0c24..4268c91d668 100644
--- a/scrapy/core/spidermw.py
+++ b/scrapy/core/spidermw.py
@@ -3,15 +3,40 @@
 
 See documentation in docs/topics/spider-middleware.rst
 """
+from itertools import chain
+
 import six
 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
 
+
 def _isiterable(possible_iterator):
     return hasattr(possible_iterator, '__iter__')
 
+
+class MutableChain:
+    """
+    Thin wrapper around itertools.chain, allowing to add iterables "in-place"
+    """
+    def __init__(self, *args):
+        self.data = chain(*args)
+
+    def extend(self, *iterables):
+        self.data = chain(self.data, *iterables)
+
+    def __iter__(self):
+        return self.data.__iter__()
+
+    def __next__(self):  # py3
+        return self.data.__next__()
+
+    def next(self):  # py2
+        return self.data.next()
+
+
 class SpiderMiddlewareManager(MiddlewareManager):
 
     component_name = 'spider middleware'
@@ -24,12 +49,10 @@ def _add_middleware(self, mw):
         super(SpiderMiddlewareManager, self)._add_middleware(mw)
         if hasattr(mw, 'process_spider_input'):
             self.methods['process_spider_input'].append(mw.process_spider_input)
-        if hasattr(mw, 'process_spider_output'):
-            self.methods['process_spider_output'].insert(0, mw.process_spider_output)
-        if hasattr(mw, 'process_spider_exception'):
-            self.methods['process_spider_exception'].insert(0, mw.process_spider_exception)
         if hasattr(mw, 'process_start_requests'):
             self.methods['process_start_requests'].insert(0, mw.process_start_requests)
+        self.methods['process_spider_output'].insert(0, getattr(mw, 'process_spider_output', None))
+        self.methods['process_spider_exception'].insert(0, getattr(mw, 'process_spider_exception', None))
 
     def scrape_response(self, scrape_func, response, request, spider):
         fname = lambda f:'%s.%s' % (
@@ -40,36 +63,71 @@ def process_spider_input(response):
             for method in self.methods['process_spider_input']:
                 try:
                     result = method(response=response, spider=spider)
-                    assert result is None, \
-                            'Middleware %s must returns None or ' \
-                            'raise an exception, got %s ' \
-                            % (fname(method), type(result))
+                    if result is not None:
+                        raise _InvalidOutput('Middleware {} must return None or raise an exception, got {}' \
+                                             .format(fname(method), type(result)))
+                except _InvalidOutput:
+                    raise
                 except:
                     return scrape_func(Failure(), request, spider)
             return scrape_func(response, request, spider)
 
-        def process_spider_exception(_failure):
+        def process_spider_exception(_failure, start_index=0):
             exception = _failure.value
-            for method in self.methods['process_spider_exception']:
+            # don't handle _InvalidOutput exception
+            if isinstance(exception, _InvalidOutput):
+                return _failure
+            for method_index, method in enumerate(self.methods['process_spider_exception'][start_index:], start=start_index):
+                if method is None:
+                    continue
                 result = method(response=response, exception=exception, spider=spider)
-                assert result is None or _isiterable(result), \
-                    'Middleware %s must returns None, or an iterable object, got %s ' % \
-                    (fname(method), type(result))
-                if result is not None:
-                    return result
+                if _isiterable(result):
+                    # stop exception handling by handing control over to the
+                    # process_spider_output chain if an iterable has been returned
+                    return process_spider_output(result, method_index+1)
+                elif result is None:
+                    continue
+                else:
+                    raise _InvalidOutput('Middleware {} must return None or an iterable, got {}' \
+                                         .format(fname(method), type(result)))
             return _failure
 
-        def process_spider_output(result):
-            for method in self.methods['process_spider_output']:
-                result = method(response=response, result=result, spider=spider)
-                assert _isiterable(result), \
-                    'Middleware %s must returns an iterable object, got %s ' % \
-                    (fname(method), type(result))
-            return result
+        def process_spider_output(result, start_index=0):
+            # items in this iterable do not need to go through the process_spider_output
+            # 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)
+
+            for method_index, method in enumerate(self.methods['process_spider_output'][start_index:], start=start_index):
+                if method is None:
+                    continue
+                # the following might fail directly if the output value is not a generator
+                try:
+                    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)))
+
+            return chain(result, recovered)
 
         dfd = mustbe_deferred(process_spider_input, response)
-        dfd.addErrback(process_spider_exception)
-        dfd.addCallback(process_spider_output)
+        dfd.addCallbacks(callback=process_spider_output, errback=process_spider_exception)
         return dfd
 
     def process_start_requests(self, start_requests, spider):
diff --git a/scrapy/exceptions.py b/scrapy/exceptions.py
index 4bcecd99479..96949bdd970 100644
--- a/scrapy/exceptions.py
+++ b/scrapy/exceptions.py
@@ -11,6 +11,13 @@ class NotConfigured(Exception):
     """Indicates a missing configuration situation"""
     pass
 
+class _InvalidOutput(TypeError):
+    """
+    Indicates an invalid value has been returned by a middleware's processing method.
+    Internal and undocumented, it should not be raised or caught by user code.
+    """
+    pass
+
 # HTTP and crawling
 
 class IgnoreRequest(Exception):
diff --git a/tests/test_downloadermiddleware.py b/tests/test_downloadermiddleware.py
index fb51392b2cb..0f420b70d2f 100644
--- a/tests/test_downloadermiddleware.py
+++ b/tests/test_downloadermiddleware.py
@@ -3,6 +3,7 @@
 
 from scrapy.http import Request, Response
 from scrapy.spiders import Spider
+from scrapy.exceptions import _InvalidOutput
 from scrapy.core.downloader.middleware import DownloaderMiddlewareManager
 from scrapy.utils.test import get_crawler
 from scrapy.utils.python import to_bytes
@@ -115,3 +116,66 @@ def process_request(self, request, spider):
 
         self.assertIs(results[0], resp)
         self.assertFalse(download_func.called)
+
+
+class ProcessRequestInvalidOutput(ManagerTestCase):
+    """Invalid return value for process_request method should raise an exception"""
+
+    def test_invalid_process_request(self):
+        req = Request('http://example.com/index.html')
+        resp = Response('http://example.com/index.html')
+
+        class InvalidProcessRequestMiddleware:
+            def process_request(self, request, spider):
+                return 1
+
+        self.mwman._add_middleware(InvalidProcessRequestMiddleware())
+        download_func = mock.MagicMock()
+        dfd = self.mwman.download(download_func, req, self.spider)
+        results = []
+        dfd.addBoth(results.append)
+        self.assertIsInstance(results[0], Failure)
+        self.assertIsInstance(results[0].value, _InvalidOutput)
+
+
+class ProcessResponseInvalidOutput(ManagerTestCase):
+    """Invalid return value for process_response method should raise an exception"""
+
+    def test_invalid_process_response(self):
+        req = Request('http://example.com/index.html')
+        resp = Response('http://example.com/index.html')
+
+        class InvalidProcessResponseMiddleware:
+            def process_response(self, request, response, spider):
+                return 1
+
+        self.mwman._add_middleware(InvalidProcessResponseMiddleware())
+        download_func = mock.MagicMock()
+        dfd = self.mwman.download(download_func, req, self.spider)
+        results = []
+        dfd.addBoth(results.append)
+        self.assertIsInstance(results[0], Failure)
+        self.assertIsInstance(results[0].value, _InvalidOutput)
+
+
+class ProcessExceptionInvalidOutput(ManagerTestCase):
+    """Invalid return value for process_exception method should raise an exception"""
+
+    def test_invalid_process_exception(self):
+        req = Request('http://example.com/index.html')
+        resp = Response('http://example.com/index.html')
+
+        class InvalidProcessExceptionMiddleware:
+            def process_request(self, request, spider):
+                raise Exception()
+
+            def process_exception(self, request, exception, spider):
+                return 1
+
+        self.mwman._add_middleware(InvalidProcessExceptionMiddleware())
+        download_func = mock.MagicMock()
+        dfd = self.mwman.download(download_func, req, self.spider)
+        results = []
+        dfd.addBoth(results.append)
+        self.assertIsInstance(results[0], Failure)
+        self.assertIsInstance(results[0].value, _InvalidOutput)
diff --git a/tests/test_spidermiddleware.py b/tests/test_spidermiddleware.py
new file mode 100644
index 00000000000..832fd3330b1
--- /dev/null
+++ b/tests/test_spidermiddleware.py
@@ -0,0 +1,102 @@
+from twisted.trial.unittest import TestCase
+from twisted.python.failure import Failure
+
+from scrapy.spiders import Spider
+from scrapy.http import Request, Response
+from scrapy.exceptions import _InvalidOutput
+from scrapy.utils.test import get_crawler
+from scrapy.core.spidermw import SpiderMiddlewareManager
+from tests import mock
+
+
+class SpiderMiddlewareTestCase(TestCase):
+
+    def setUp(self):
+        self.request = Request('http://example.com/index.html')
+        self.response = Response(self.request.url, request=self.request)
+        self.crawler = get_crawler(Spider)
+        self.spider = self.crawler._create_spider('foo')
+        self.mwman = SpiderMiddlewareManager.from_crawler(self.crawler)
+
+    def _scrape_response(self):
+        """Execute spider mw manager's scrape_response method and return the result.
+        Raise exception in case of failure.
+        """
+        scrape_func = mock.MagicMock()
+        dfd = self.mwman.scrape_response(scrape_func, self.response, self.request, self.spider)
+        # catch deferred result and return the value
+        results = []
+        dfd.addBoth(results.append)
+        self._wait(dfd)
+        ret = results[0]
+        return ret
+
+
+class ProcessSpiderInputInvalidOutput(SpiderMiddlewareTestCase):
+    """Invalid return value for process_spider_input method"""
+
+    def test_invalid_process_spider_input(self):
+
+        class InvalidProcessSpiderInputMiddleware:
+            def process_spider_input(self, response, spider):
+                return 1
+
+        self.mwman._add_middleware(InvalidProcessSpiderInputMiddleware())
+        result = self._scrape_response()
+        self.assertIsInstance(result, Failure)
+        self.assertIsInstance(result.value, _InvalidOutput)
+
+
+class ProcessSpiderOutputInvalidOutput(SpiderMiddlewareTestCase):
+    """Invalid return value for process_spider_output method"""
+
+    def test_invalid_process_spider_output(self):
+
+        class InvalidProcessSpiderOutputMiddleware:
+            def process_spider_output(self, response, result, spider):
+                return 1
+
+        self.mwman._add_middleware(InvalidProcessSpiderOutputMiddleware())
+        result = self._scrape_response()
+        self.assertIsInstance(result, Failure)
+        self.assertIsInstance(result.value, _InvalidOutput)
+
+
+class ProcessSpiderExceptionInvalidOutput(SpiderMiddlewareTestCase):
+    """Invalid return value for process_spider_exception method"""
+
+    def test_invalid_process_spider_exception(self):
+
+        class InvalidProcessSpiderOutputExceptionMiddleware:
+            def process_spider_exception(self, response, exception, spider):
+                return 1
+
+        class RaiseExceptionProcessSpiderOutputMiddleware:
+            def process_spider_output(self, response, result, spider):
+                raise Exception()
+
+        self.mwman._add_middleware(InvalidProcessSpiderOutputExceptionMiddleware())
+        self.mwman._add_middleware(RaiseExceptionProcessSpiderOutputMiddleware())
+        result = self._scrape_response()
+        self.assertIsInstance(result, Failure)
+        self.assertIsInstance(result.value, _InvalidOutput)
+
+
+class ProcessSpiderExceptionReRaise(SpiderMiddlewareTestCase):
+    """Re raise the exception by returning None"""
+
+    def test_process_spider_exception_return_none(self):
+
+        class ProcessSpiderExceptionReturnNoneMiddleware:
+            def process_spider_exception(self, response, exception, spider):
+                return None
+
+        class RaiseExceptionProcessSpiderOutputMiddleware:
+            def process_spider_output(self, response, result, spider):
+                1/0
+
+        self.mwman._add_middleware(ProcessSpiderExceptionReturnNoneMiddleware())
+        self.mwman._add_middleware(RaiseExceptionProcessSpiderOutputMiddleware())
+        result = self._scrape_response()
+        self.assertIsInstance(result, Failure)
+        self.assertIsInstance(result.value, ZeroDivisionError)
diff --git a/tests/test_spidermiddleware_output_chain.py b/tests/test_spidermiddleware_output_chain.py
new file mode 100644
index 00000000000..6f8727a1580
--- /dev/null
+++ b/tests/test_spidermiddleware_output_chain.py
@@ -0,0 +1,380 @@
+
+from testfixtures import LogCapture
+from twisted.trial.unittest import TestCase
+from twisted.internet import defer
+
+from scrapy import Spider, Request
+from scrapy.utils.test import get_crawler
+from tests.mockserver import MockServer
+from tests.spiders import MockServerSpider
+
+
+class LogExceptionMiddleware:
+    def process_spider_exception(self, response, exception, spider):
+        spider.logger.info('Middleware: %s exception caught', exception.__class__.__name__)
+        return None
+
+
+# ================================================================================
+# (0) recover from an exception on a spider callback
+class RecoverySpider(Spider):
+    name = 'RecoverySpider'
+    custom_settings = {
+        'SPIDER_MIDDLEWARES': {
+            __name__ + '.RecoveryMiddleware': 10,
+        },
+    }
+
+    def start_requests(self):
+        yield Request(self.mockserver.url('/status?n=200'))
+
+    def parse(self, response):
+        yield {'test': 1}
+        self.logger.info('DONT_FAIL: %s', response.meta.get('dont_fail'))
+        if not response.meta.get('dont_fail'):
+            raise TabError()
+
+class RecoveryMiddleware:
+    def process_spider_exception(self, response, exception, spider):
+        spider.logger.info('Middleware: %s exception caught', exception.__class__.__name__)
+        return [
+            {'from': 'process_spider_exception'},
+            Request(response.url, meta={'dont_fail': True}, dont_filter=True),
+        ]
+
+
+# ================================================================================
+# (1) exceptions from a spider middleware's process_spider_input method
+class FailProcessSpiderInputMiddleware:
+    def process_spider_input(self, response, spider):
+        spider.logger.info('Middleware: will raise IndexError')
+        raise IndexError()
+
+class ProcessSpiderInputSpiderWithoutErrback(Spider):
+    name = 'ProcessSpiderInputSpiderWithoutErrback'
+    custom_settings = {
+        'SPIDER_MIDDLEWARES': {
+            # spider
+            __name__ + '.LogExceptionMiddleware': 10,
+            __name__ + '.FailProcessSpiderInputMiddleware': 8,
+            __name__ + '.LogExceptionMiddleware': 6,
+            # engine
+        }
+    }
+
+    def start_requests(self):
+        yield Request(url=self.mockserver.url('/status?n=200'), callback=self.parse)
+
+    def parse(self, response):
+        return {'from': 'callback'}
+
+
+class ProcessSpiderInputSpiderWithErrback(ProcessSpiderInputSpiderWithoutErrback):
+    name = 'ProcessSpiderInputSpiderWithErrback'
+
+    def start_requests(self):
+        yield Request(url=self.mockserver.url('/status?n=200'), callback=self.parse, errback=self.errback)
+
+    def errback(self, failure):
+        self.logger.info('Got a Failure on the Request errback')
+        return {'from': 'errback'}
+
+
+# ================================================================================
+# (2) exceptions from a spider callback (generator)
+class GeneratorCallbackSpider(Spider):
+    name = 'GeneratorCallbackSpider'
+    custom_settings = {
+        'SPIDER_MIDDLEWARES': {
+            __name__ + '.LogExceptionMiddleware': 10,
+        },
+    }
+
+    def start_requests(self):
+        yield Request(self.mockserver.url('/status?n=200'))
+
+    def parse(self, response):
+        yield {'test': 1}
+        yield {'test': 2}
+        raise ImportError()
+
+
+# ================================================================================
+# (3) exceptions from a spider callback (not a generator)
+class NotGeneratorCallbackSpider(Spider):
+    name = 'NotGeneratorCallbackSpider'
+    custom_settings = {
+        'SPIDER_MIDDLEWARES': {
+            __name__ + '.LogExceptionMiddleware': 10,
+        },
+    }
+
+    def start_requests(self):
+        yield Request(self.mockserver.url('/status?n=200'))
+
+    def parse(self, response):
+        return [{'test': 1}, {'test': 1/0}]
+
+
+# ================================================================================
+# (4) exceptions from a middleware process_spider_output method (generator)
+class GeneratorOutputChainSpider(Spider):
+    name = 'GeneratorOutputChainSpider'
+    custom_settings = {
+        'SPIDER_MIDDLEWARES': {
+            __name__ + '.GeneratorFailMiddleware': 10,
+            __name__ + '.GeneratorDoNothingAfterFailureMiddleware': 8,
+            __name__ + '.GeneratorRecoverMiddleware': 5,
+            __name__ + '.GeneratorDoNothingAfterRecoveryMiddleware': 3,
+        },
+    }
+
+    def start_requests(self):
+        yield Request(self.mockserver.url('/status?n=200'))
+
+    def parse(self, response):
+        yield {'processed': ['parse-first-item']}
+        yield {'processed': ['parse-second-item']}
+
+
+class _GeneratorDoNothingMiddleware:
+    def process_spider_output(self, response, result, spider):
+        for r in result:
+            r['processed'].append('{}.process_spider_output'.format(self.__class__.__name__))
+            yield r
+
+    def process_spider_exception(self, response, exception, spider):
+        method = '{}.process_spider_exception'.format(self.__class__.__name__)
+        spider.logger.info('%s: %s caught', method, exception.__class__.__name__)
+        return None
+
+
+class GeneratorFailMiddleware:
+    def process_spider_output(self, response, result, spider):
+        for r in result:
+            r['processed'].append('{}.process_spider_output'.format(self.__class__.__name__))
+            yield r
+            raise LookupError()
+    
+    def process_spider_exception(self, response, exception, spider):
+        method = '{}.process_spider_exception'.format(self.__class__.__name__)
+        spider.logger.info('%s: %s caught', method, exception.__class__.__name__)
+        yield {'processed': [method]}
+
+
+class GeneratorDoNothingAfterFailureMiddleware(_GeneratorDoNothingMiddleware):
+    pass
+
+
+class GeneratorRecoverMiddleware:
+    def process_spider_output(self, response, result, spider):
+        for r in result:
+            r['processed'].append('{}.process_spider_output'.format(self.__class__.__name__))
+            yield r
+
+    def process_spider_exception(self, response, exception, spider):
+        method = '{}.process_spider_exception'.format(self.__class__.__name__)
+        spider.logger.info('%s: %s caught', method, exception.__class__.__name__)
+        yield {'processed': [method]}
+
+class GeneratorDoNothingAfterRecoveryMiddleware(_GeneratorDoNothingMiddleware):
+    pass
+
+
+# ================================================================================
+# (5) exceptions from a middleware process_spider_output method (not generator)
+class NotGeneratorOutputChainSpider(Spider):
+    name = 'NotGeneratorOutputChainSpider'
+    custom_settings = {
+        'SPIDER_MIDDLEWARES': {
+            __name__ + '.NotGeneratorFailMiddleware': 10,
+            __name__ + '.NotGeneratorDoNothingAfterFailureMiddleware': 8,
+            __name__ + '.NotGeneratorRecoverMiddleware': 5,
+            __name__ + '.NotGeneratorDoNothingAfterRecoveryMiddleware': 3,
+        },
+    }
+
+    def start_requests(self):
+        return [Request(self.mockserver.url('/status?n=200'))]
+
+    def parse(self, response):
+        return [{'processed': ['parse-first-item']}, {'processed': ['parse-second-item']}]
+
+
+class _NotGeneratorDoNothingMiddleware:
+    def process_spider_output(self, response, result, spider):
+        out = []
+        for r in result:
+            r['processed'].append('{}.process_spider_output'.format(self.__class__.__name__))
+            out.append(r)
+        return out
+
+    def process_spider_exception(self, response, exception, spider):
+        method = '{}.process_spider_exception'.format(self.__class__.__name__)
+        spider.logger.info('%s: %s caught', method, exception.__class__.__name__)
+        return None
+
+
+class NotGeneratorFailMiddleware:
+    def process_spider_output(self, response, result, spider):
+        out = []
+        for r in result:
+            r['processed'].append('{}.process_spider_output'.format(self.__class__.__name__))
+            out.append(r)
+        raise ReferenceError()
+        return out
+
+    def process_spider_exception(self, response, exception, spider):
+        method = '{}.process_spider_exception'.format(self.__class__.__name__)
+        spider.logger.info('%s: %s caught', method, exception.__class__.__name__)
+        return [{'processed': [method]}]
+
+
+class NotGeneratorDoNothingAfterFailureMiddleware(_NotGeneratorDoNothingMiddleware):
+    pass
+
+
+class NotGeneratorRecoverMiddleware:
+    def process_spider_output(self, response, result, spider):
+        out = []
+        for r in result:
+            r['processed'].append('{}.process_spider_output'.format(self.__class__.__name__))
+            out.append(r)
+        return out
+
+    def process_spider_exception(self, response, exception, spider):
+        method = '{}.process_spider_exception'.format(self.__class__.__name__)
+        spider.logger.info('%s: %s caught', method, exception.__class__.__name__)
+        return [{'processed': [method]}]
+
+class NotGeneratorDoNothingAfterRecoveryMiddleware(_NotGeneratorDoNothingMiddleware):
+    pass
+
+
+# ================================================================================
+class TestSpiderMiddleware(TestCase):
+    @classmethod
+    def setUpClass(cls):
+        cls.mockserver = MockServer()
+        cls.mockserver.__enter__()
+
+    @classmethod
+    def tearDownClass(cls):
+        cls.mockserver.__exit__(None, None, None)
+    
+    @defer.inlineCallbacks
+    def crawl_log(self, spider):
+        crawler = get_crawler(spider)
+        with LogCapture() as log:
+            yield crawler.crawl(mockserver=self.mockserver)
+        raise defer.returnValue(log)
+
+    @defer.inlineCallbacks
+    def test_recovery(self):
+        """
+        (0) Recover from an exception in a spider callback. The final item count should be 3
+        (one yielded from the callback method before the exception is raised, one directly
+        from the recovery middleware and one from the spider when processing the request that
+        was enqueued from the recovery middleware)
+        """
+        log = yield self.crawl_log(RecoverySpider)
+        self.assertIn("Middleware: TabError exception caught", str(log))
+        self.assertEqual(str(log).count("Middleware: TabError exception caught"), 1)
+        self.assertIn("'item_scraped_count': 3", str(log))
+
+    @defer.inlineCallbacks
+    def test_process_spider_input_without_errback(self):
+        """
+        (1.1) An exception from the process_spider_input chain should be caught by the
+        process_spider_exception chain from the start if the Request has no errback
+        """
+        log1 = yield self.crawl_log(ProcessSpiderInputSpiderWithoutErrback)
+        self.assertIn("Middleware: will raise IndexError", str(log1))
+        self.assertIn("Middleware: IndexError exception caught", str(log1))
+
+    @defer.inlineCallbacks
+    def test_process_spider_input_with_errback(self):
+        """
+        (1.2) An exception from the process_spider_input chain should not be caught by the
+        process_spider_exception chain if the Request has an errback
+        """
+        log1 = yield self.crawl_log(ProcessSpiderInputSpiderWithErrback)
+        self.assertNotIn("Middleware: IndexError exception caught", str(log1))
+        self.assertIn("Middleware: will raise IndexError", str(log1))
+        self.assertIn("Got a Failure on the Request errback", str(log1))
+        self.assertIn("{'from': 'errback'}", str(log1))
+        self.assertNotIn("{'from': 'callback'}", str(log1))
+        self.assertIn("'item_scraped_count': 1", str(log1))
+    
+    @defer.inlineCallbacks
+    def test_generator_callback(self):
+        """
+        (2) An exception from a spider callback (returning a generator) should
+        be caught by the process_spider_exception chain. Items yielded before the
+        exception is raised should be processed normally.
+        """
+        log2 = yield self.crawl_log(GeneratorCallbackSpider)
+        self.assertIn("Middleware: ImportError exception caught", str(log2))
+        self.assertIn("'item_scraped_count': 2", str(log2))
+    
+    @defer.inlineCallbacks
+    def test_not_a_generator_callback(self):
+        """
+        (3) An exception from a spider callback (returning a list) should
+        be caught by the process_spider_exception chain. No items should be processed.
+        """
+        log3 = yield self.crawl_log(NotGeneratorCallbackSpider)
+        self.assertIn("Middleware: ZeroDivisionError exception caught", str(log3))
+        self.assertNotIn("item_scraped_count", str(log3))
+
+    @defer.inlineCallbacks
+    def test_generator_output_chain(self):
+        """
+        (4) An exception from a middleware's process_spider_output method should be sent
+        to the process_spider_exception method from the next middleware in the chain.
+        The result of the recovery by the process_spider_exception method should be handled
+        by the process_spider_output method from the next middleware.
+        The final item count should be 2 (one from the spider callback and one from the
+        process_spider_exception chain)
+        """
+        log4 = yield self.crawl_log(GeneratorOutputChainSpider)
+        self.assertIn("'item_scraped_count': 2", str(log4))
+        self.assertIn("GeneratorRecoverMiddleware.process_spider_exception: LookupError caught", str(log4))
+        self.assertIn("GeneratorDoNothingAfterFailureMiddleware.process_spider_exception: LookupError caught", str(log4))
+        self.assertNotIn("GeneratorFailMiddleware.process_spider_exception: LookupError caught", str(log4))
+        self.assertNotIn("GeneratorDoNothingAfterRecoveryMiddleware.process_spider_exception: LookupError caught", str(log4))
+        item_from_callback = {'processed': [
+            'parse-first-item',
+            'GeneratorFailMiddleware.process_spider_output',
+            'GeneratorDoNothingAfterFailureMiddleware.process_spider_output',
+            'GeneratorRecoverMiddleware.process_spider_output',
+            'GeneratorDoNothingAfterRecoveryMiddleware.process_spider_output']}
+        item_recovered = {'processed': [
+            'GeneratorRecoverMiddleware.process_spider_exception',
+            'GeneratorDoNothingAfterRecoveryMiddleware.process_spider_output']}
+        self.assertIn(str(item_from_callback), str(log4))
+        self.assertIn(str(item_recovered), str(log4))
+        self.assertNotIn('parse-second-item', str(log4))
+
+    @defer.inlineCallbacks
+    def test_not_a_generator_output_chain(self):
+        """
+        (5) An exception from a middleware's process_spider_output method should be sent
+        to the process_spider_exception method from the next middleware in the chain.
+        The result of the recovery by the process_spider_exception method should be handled
+        by the process_spider_output method from the next middleware.
+        The final item count should be 1 (from the process_spider_exception chain, the items
+        from the spider callback are lost)
+        """
+        log5 = yield self.crawl_log(NotGeneratorOutputChainSpider)
+        self.assertIn("'item_scraped_count': 1", str(log5))
+        self.assertIn("GeneratorRecoverMiddleware.process_spider_exception: ReferenceError caught", str(log5))
+        self.assertIn("GeneratorDoNothingAfterFailureMiddleware.process_spider_exception: ReferenceError caught", str(log5))
+        self.assertNotIn("GeneratorFailMiddleware.process_spider_exception: ReferenceError caught", str(log5))
+        self.assertNotIn("GeneratorDoNothingAfterRecoveryMiddleware.process_spider_exception: ReferenceError caught", str(log5))
+        item_recovered = {'processed': [
+            'NotGeneratorRecoverMiddleware.process_spider_exception',
+            'NotGeneratorDoNothingAfterRecoveryMiddleware.process_spider_output']}
+        self.assertIn(str(item_recovered), str(log5))
+        self.assertNotIn('parse-first-item', str(log5))
+        self.assertNotIn('parse-second-item', str(log5))


Code review comments:
diff_hunk: @@ -3,15 +3,40 @@
 
 See documentation in docs/topics/spider-middleware.rst
 """
+from itertools import chain
+
 import six
 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
 
+
 def _isiterable(possible_iterator):
     return hasattr(possible_iterator, '__iter__')
 
+
+class MutableChain:
path: scrapy/core/spidermw.py
text: @user1:
would you mind moving it to scrapy.utils.python?

@user1:
Also, as we support Python 2, it is clearner to inherit it from object.

diff_hunk: @@ -3,15 +3,40 @@
 
 See documentation in docs/topics/spider-middleware.rst
 """
+from itertools import chain
+
 import six
 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
 
+
 def _isiterable(possible_iterator):
     return hasattr(possible_iterator, '__iter__')
 
+
+class MutableChain:
+    """
+    Thin wrapper around itertools.chain, allowing to add iterables "in-place"
+    """
+    def __init__(self, *args):
+        self.data = chain(*args)
+
+    def extend(self, *iterables):
+        self.data = chain(self.data, *iterables)
+
+    def __iter__(self):
+        return self.data.__iter__()
+
+    def __next__(self):  # py3
+        return self.data.__next__()
+
+    def next(self):  # py2
path: scrapy/core/spidermw.py
text: @user1:
I wonder if you can make `def next` an alias `next = __next__`, and implement `__next__` as `next(self.data)`. `next` built-in function calls `.next()` in Python 2 and `__next__()` in Python 3 - see e.g. https://docs.python.org/2/library/functions.html#next

diff_hunk: @@ -40,36 +63,71 @@ def process_spider_input(response):
             for method in self.methods['process_spider_input']:
                 try:
                     result = method(response=response, spider=spider)
-                    assert result is None, \
-                            'Middleware %s must returns None or ' \
-                            'raise an exception, got %s ' \
-                            % (fname(method), type(result))
+                    if result is not None:
+                        raise _InvalidOutput('Middleware {} must return None or raise an exception, got {}' \
+                                             .format(fname(method), type(result)))
+                except _InvalidOutput:
+                    raise
                 except:
                     return scrape_func(Failure(), request, spider)
             return scrape_func(response, request, spider)
 
-        def process_spider_exception(_failure):
+        def process_spider_exception(_failure, start_index=0):
             exception = _failure.value
-            for method in self.methods['process_spider_exception']:
+            # don't handle _InvalidOutput exception
+            if isinstance(exception, _InvalidOutput):
+                return _failure
+            for method_index, method in enumerate(self.methods['process_spider_exception'][start_index:], start=start_index):
path: scrapy/core/spidermw.py
text: @user1:
could you please extract `self.methods['process_spider_exception'][start_index:]` to a variable, to make the line shorter?

@user1:
the same in process_spider_output