diff --git a/scrapy/pipelines/media.py b/scrapy/pipelines/media.py index 404bbf5bf..95dca9a3f 100644 --- a/scrapy/pipelines/media.py +++ b/scrapy/pipelines/media.py @@ -3,7 +3,7 @@ from __future__ import print_function import functools import logging from collections import defaultdict -from twisted.internet.defer import Deferred, DeferredList +from twisted.internet.defer import Deferred, DeferredList, _DefGen_Return from twisted.python.failure import Failure from scrapy.settings import Settings @@ -139,6 +139,30 @@ class MediaPipeline(object): result.cleanFailure() result.frames = [] result.stack = None + + # This code fixes a memory leak by avoiding to keep references to + # the Request and Response objects on the Media Pipeline cache. + # + # Twisted inline callbacks pass return values using the function + # twisted.internet.defer.returnValue, which encapsulates the return + # value inside a _DefGen_Return base exception. + # + # What happens when the media_downloaded callback raises another + # exception, for example a FileException('download-error') when + # the Response status code is not 200 OK, is that it stores the + # _DefGen_Return exception on the FileException context. + # + # To avoid keeping references to the Response and therefore Request + # objects on the Media Pipeline cache, we should wipe the context of + # the exception encapsulated by the Twisted Failure when its a + # _DefGen_Return instance. + # + # This problem does not occur in Python 2.7 since we don't have + # Exception Chaining (https://www.python.org/dev/peps/pep-3134/). + context = getattr(result.value, '__context__', None) + if isinstance(context, _DefGen_Return): + setattr(result.value, '__context__', None) + info.downloading.remove(fp) info.downloaded[fp] = result # cache result for wad in info.waiting.pop(fp): diff --git a/tests/test_pipeline_media.py b/tests/test_pipeline_media.py index 5f6a6d9e6..28e39cefa 100644 --- a/tests/test_pipeline_media.py +++ b/tests/test_pipeline_media.py @@ -1,15 +1,19 @@ from __future__ import print_function + +import sys + from testfixtures import LogCapture from twisted.trial import unittest from twisted.python.failure import Failure from twisted.internet import reactor -from twisted.internet.defer import Deferred, inlineCallbacks +from twisted.internet.defer import Deferred, inlineCallbacks, returnValue from scrapy.http import Request, Response from scrapy.settings import Settings from scrapy.spiders import Spider from scrapy.utils.request import request_fingerprint from scrapy.pipelines.media import MediaPipeline +from scrapy.pipelines.files import FileException from scrapy.utils.log import failure_to_exc_info from scrapy.utils.signal import disconnect_all from scrapy import signals @@ -90,6 +94,77 @@ class BaseMediaPipelineTestCase(unittest.TestCase): self.pipe._modify_media_request(request) assert request.meta == {'handle_httpstatus_all': True} + def test_should_remove_req_res_references_before_caching_the_results(self): + """Regression test case to prevent a memory leak in the Media Pipeline. + + The memory leak is triggered when an exception is raised when a Response + scheduled by the Media Pipeline is being returned. For example, when a + FileException('download-error') is raised because the Response status + code is not 200 OK. + + It happens because we are keeping a reference to the Response object + inside the FileException context. This is caused by the way Twisted + return values from inline callbacks. It raises a custom exception + encapsulating the original return value. + + The solution is to remove the exception context when this context is a + _DefGen_Return instance, the BaseException used by Twisted to pass the + returned value from those inline callbacks. + + Maybe there's a better and more reliable way to test the case described + here, but it would be more complicated and involve running - or at least + mocking - some async steps from the Media Pipeline. The current test + case is simple and detects the problem very fast. On the other hand, it + would not detect another kind of leak happening due to old object + references being kept inside the Media Pipeline cache. + + This problem does not occur in Python 2.7 since we don't have Exception + Chaining (https://www.python.org/dev/peps/pep-3134/). + """ + # Create sample pair of Request and Response objects + request = Request('http://url') + response = Response('http://url', body=b'', request=request) + + # Simulate the Media Pipeline behavior to produce a Twisted Failure + try: + # Simulate a Twisted inline callback returning a Response + # The returnValue method raises an exception encapsulating the value + returnValue(response) + except BaseException as exc: + def_gen_return_exc = exc + try: + # Simulate the media_downloaded callback raising a FileException + # This usually happens when the status code is not 200 OK + raise FileException('download-error') + except Exception as exc: + file_exc = exc + # Simulate Twisted capturing the FileException + # It encapsulates the exception inside a Twisted Failure + failure = Failure(file_exc) + + # The Failure should encapsulate a FileException ... + self.assertEqual(failure.value, file_exc) + # ... and if we're running on Python 3 ... + if sys.version_info.major >= 3: + # ... it should have the returnValue exception set as its context + self.assertEqual(failure.value.__context__, def_gen_return_exc) + + # Let's calculate the request fingerprint and fake some runtime data... + fp = request_fingerprint(request) + info = self.pipe.spiderinfo + info.downloading.add(fp) + info.waiting[fp] = [] + + # When calling the method that caches the Request's result ... + self.pipe._cache_result_and_execute_waiters(failure, fp, info) + # ... it should store the Twisted Failure ... + self.assertEqual(info.downloaded[fp], failure) + # ... encapsulating the original FileException ... + self.assertEqual(info.downloaded[fp].value, file_exc) + # ... but it should not store the returnValue exception on its context + context = getattr(info.downloaded[fp].value, '__context__', None) + self.assertIsNone(context) + class MockedMediaPipeline(MediaPipeline):