Repository: scrapy/scrapy
Pull request #3813 (2019-06-05 21:23:43)

Fix a memory leak on the Media Pipeline (Files and Images)
We're storing exceptions captured by Twisted on the media pipeline
cache, but we're also using the defer.returnValue method with our
own methods decorated with @defer.inlineCallbacks.

The defer.returnValue method passes returned values forward by
throwing a defer._DefGen_Return exception, which in its turn
extends the BaseException class and is captured by Twisted.

This way, the latest exception stored in the Failure's object may
also have an HtmlResponse object in its `__context__` attribute. As
the Response object also keeps track of the Request object that
has originated it, you could figure it out how many RAM we're
wasting here.

This could easily lead to a Memory Leak problem when running
spiders with Media Pipeline enabled and a particular Request set
that tends to raise a significant number of exceptions.

Example triggers:
- media requests with 404 status responses
- userland exceptions coming from custom middlewares
- etc.


Patch:
diff --git a/docs/conf.py b/docs/conf.py
index 832626f6b..bf222b361 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -224,4 +224,17 @@ linkcheck_ignore = [
 # Options for the Coverage extension
 # ----------------------------------
 coverage_ignore_pyobjects = [
+    # Contract’s add_pre_hook and add_post_hook are not documented because
+    # they should be transparent to contract developers, for whom pre_hook and
+    # post_hook should be the actual concern.
+    r'\bContract\.add_(pre|post)_hook$',
+
+    # ContractsManager is an internal class, developers are not expected to
+    # interact with it directly in any way.
+    r'\bContractsManager\b$',
+
+    # For default contracts we only want to document their general purpose in
+    # their constructor, the methods they reimplement to achieve that purpose
+    # should be irrelevant to developers using those contracts.
+    r'\w+Contract\.(adjust_request_args|(pre|post)_process)$',
 ]
diff --git a/docs/topics/contracts.rst b/docs/topics/contracts.rst
index 70f20d4ed..9337375bb 100644
--- a/docs/topics/contracts.rst
+++ b/docs/topics/contracts.rst
@@ -120,3 +120,23 @@ get the failures pretty printed::
             for header in self.args:
                 if header not in response.headers:
                     raise ContractFail('X-CustomHeader not present')
+
+
+Detecting check runs
+====================
+
+When ``scrapy check`` is running, the ``SCRAPY_CHECK`` environment variable is
+set to the ``true`` string. You can use `os.environ`_ to perform any change to
+your spiders or your settings when ``scrapy check`` is used::
+
+    import os
+    import scrapy
+
+    class ExampleSpider(scrapy.Spider):
+        name = 'example'
+
+        def __init__(self):
+            if os.environ.get('SCRAPY_CHECK'):
+                pass  # Do some scraper adjustments when a check is running
+
+.. _os.environ: https://docs.python.org/3/library/os.html#os.environ
diff --git a/scrapy/commands/check.py b/scrapy/commands/check.py
index b8a9ef989..ab73e85e7 100644
--- a/scrapy/commands/check.py
+++ b/scrapy/commands/check.py
@@ -6,7 +6,7 @@ from unittest import TextTestRunner, TextTestResult as _TextTestResult
 
 from scrapy.commands import ScrapyCommand
 from scrapy.contracts import ContractsManager
-from scrapy.utils.misc import load_object
+from scrapy.utils.misc import load_object, set_environ
 from scrapy.utils.conf import build_component_list
 
 
@@ -68,16 +68,17 @@ class Command(ScrapyCommand):
 
         spider_loader = self.crawler_process.spider_loader
 
-        for spidername in args or spider_loader.list():
-            spidercls = spider_loader.load(spidername)
-            spidercls.start_requests = lambda s: conman.from_spider(s, result)
-
-            tested_methods = conman.tested_methods_from_spidercls(spidercls)
-            if opts.list:
-                for method in tested_methods:
-                    contract_reqs[spidercls.name].append(method)
-            elif tested_methods:
-                self.crawler_process.crawl(spidercls)
+        with set_environ(SCRAPY_CHECK='true'):
+            for spidername in args or spider_loader.list():
+                spidercls = spider_loader.load(spidername)
+                spidercls.start_requests = lambda s: conman.from_spider(s, result)
+
+                tested_methods = conman.tested_methods_from_spidercls(spidercls)
+                if opts.list:
+                    for method in tested_methods:
+                        contract_reqs[spidercls.name].append(method)
+                elif tested_methods:
+                    self.crawler_process.crawl(spidercls)
 
         # start checks
         if opts.list:
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/scrapy/settings/default_settings.py b/scrapy/settings/default_settings.py
index 9986827d8..2afa7b321 100644
--- a/scrapy/settings/default_settings.py
+++ b/scrapy/settings/default_settings.py
@@ -238,7 +238,7 @@ REFERRER_POLICY = 'scrapy.spidermiddlewares.referer.DefaultReferrerPolicy'
 
 RETRY_ENABLED = True
 RETRY_TIMES = 2  # initial response + 2 retries = 3 requests
-RETRY_HTTP_CODES = [500, 502, 503, 504, 522, 524, 408]
+RETRY_HTTP_CODES = [500, 502, 503, 504, 522, 524, 408, 429]
 RETRY_PRIORITY_ADJUST = -1
 
 ROBOTSTXT_OBEY = False
diff --git a/scrapy/utils/misc.py b/scrapy/utils/misc.py
index ddaa7f7bf..f638adb25 100644
--- a/scrapy/utils/misc.py
+++ b/scrapy/utils/misc.py
@@ -1,6 +1,8 @@
 """Helper functions which don't fit anywhere else"""
+import os
 import re
 import hashlib
+from contextlib import contextmanager
 from importlib import import_module
 from pkgutil import iter_modules
 
@@ -142,3 +144,21 @@ def create_instance(objcls, settings, crawler, *args, **kwargs):
         return objcls.from_settings(settings, *args, **kwargs)
     else:
         return objcls(*args, **kwargs)
+
+
+@contextmanager
+def set_environ(**kwargs):
+    """Temporarily set environment variables inside the context manager and
+    fully restore previous environment afterwards
+    """
+
+    original_env = {k: os.environ.get(k) for k in kwargs}
+    os.environ.update(kwargs)
+    try:
+        yield
+    finally:
+        for k, v in original_env.items():
+            if v is None:
+                del os.environ[k]
+            else:
+                os.environ[k] = v
diff --git a/scrapy/utils/reqser.py b/scrapy/utils/reqser.py
index 959dddbd5..3c463cfed 100644
--- a/scrapy/utils/reqser.py
+++ b/scrapy/utils/reqser.py
@@ -67,6 +67,20 @@ def request_from_dict(d, spider=None):
         flags=d.get('flags'))
 
 
+def _is_private_method(name):
+    return name.startswith('__') and not name.endswith('__')
+
+
+def _mangle_private_name(obj, func, name):
+    qualname = getattr(func, '__qualname__', None)
+    if qualname is None:
+        classname = obj.__class__.__name__.lstrip('_')
+        return '_%s%s' % (classname, name)
+    else:
+        splits = qualname.split('.')
+        return '_%s%s' % (splits[-2], splits[-1])
+
+
 def _find_method(obj, func):
     if obj:
         try:
@@ -75,7 +89,10 @@ def _find_method(obj, func):
             pass
         else:
             if func_self is obj:
-                return six.get_method_function(func).__name__
+                name = six.get_method_function(func).__name__
+                if _is_private_method(name):
+                    return _mangle_private_name(obj, func, name)
+                return name
     raise ValueError("Function %s is not a method of: %s" % (func, obj))
 
 


Test patch:
diff --git a/tests/test_downloadermiddleware.py b/tests/test_downloadermiddleware.py
index 0f420b70d..03564e748 100644
--- a/tests/test_downloadermiddleware.py
+++ b/tests/test_downloadermiddleware.py
@@ -123,7 +123,6 @@ class ProcessRequestInvalidOutput(ManagerTestCase):
 
     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):
@@ -143,7 +142,6 @@ class ProcessResponseInvalidOutput(ManagerTestCase):
 
     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):
@@ -163,7 +161,6 @@ class ProcessExceptionInvalidOutput(ManagerTestCase):
 
     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):
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):
 
diff --git a/tests/test_utils_misc/__init__.py b/tests/test_utils_misc/__init__.py
index fcb7772ab..e109d5343 100644
--- a/tests/test_utils_misc/__init__.py
+++ b/tests/test_utils_misc/__init__.py
@@ -3,12 +3,13 @@ import os
 import unittest
 
 from scrapy.item import Item, Field
-from scrapy.utils.misc import arg_to_iter, create_instance, load_object, walk_modules
+from scrapy.utils.misc import arg_to_iter, create_instance, load_object, set_environ, walk_modules
 
 from tests import mock
 
 __doctests__ = ['scrapy.utils.misc']
 
+
 class UtilsMiscTestCase(unittest.TestCase):
 
     def test_load_object(self):
@@ -130,5 +131,18 @@ class UtilsMiscTestCase(unittest.TestCase):
         with self.assertRaises(ValueError):
             create_instance(m, None, None)
 
+    def test_set_environ(self):
+        assert os.environ.get('some_test_environ') is None
+        with set_environ(some_test_environ='test_value'):
+            assert os.environ.get('some_test_environ') == 'test_value'
+        assert os.environ.get('some_test_environ') is None
+
+        os.environ['some_test_environ'] = 'test'
+        assert os.environ.get('some_test_environ') == 'test'
+        with set_environ(some_test_environ='test_value'):
+            assert os.environ.get('some_test_environ') == 'test_value'
+        assert os.environ.get('some_test_environ') == 'test'
+
+
 if __name__ == "__main__":
     unittest.main()
diff --git a/tests/test_utils_reqser.py b/tests/test_utils_reqser.py
index dcc070b8f..e5a09dcf1 100644
--- a/tests/test_utils_reqser.py
+++ b/tests/test_utils_reqser.py
@@ -1,9 +1,12 @@
 # -*- coding: utf-8 -*-
 import unittest
+import sys
+
+import six
 
 from scrapy.http import Request, FormRequest
 from scrapy.spiders import Spider
-from scrapy.utils.reqser import request_to_dict, request_from_dict
+from scrapy.utils.reqser import request_to_dict, request_from_dict, _is_private_method, _mangle_private_name
 
 
 class RequestSerializationTest(unittest.TestCase):
@@ -68,6 +71,56 @@ class RequestSerializationTest(unittest.TestCase):
                     errback=self.spider.handle_error)
         self._assert_serializes_ok(r, spider=self.spider)
 
+    def test_private_callback_serialization(self):
+        r = Request("http://www.example.com",
+                    callback=self.spider._TestSpider__parse_item_private,
+                    errback=self.spider.handle_error)
+        self._assert_serializes_ok(r, spider=self.spider)
+
+    def test_mixin_private_callback_serialization(self):
+        if sys.version_info[0] < 3:
+            return
+        r = Request("http://www.example.com",
+                    callback=self.spider._TestSpiderMixin__mixin_callback,
+                    errback=self.spider.handle_error)
+        self._assert_serializes_ok(r, spider=self.spider)
+
+    def test_private_callback_name_matching(self):
+        self.assertTrue(_is_private_method('__a'))
+        self.assertTrue(_is_private_method('__a_'))
+        self.assertTrue(_is_private_method('__a_a'))
+        self.assertTrue(_is_private_method('__a_a_'))
+        self.assertTrue(_is_private_method('__a__a'))
+        self.assertTrue(_is_private_method('__a__a_'))
+        self.assertTrue(_is_private_method('__a___a'))
+        self.assertTrue(_is_private_method('__a___a_'))
+        self.assertTrue(_is_private_method('___a'))
+        self.assertTrue(_is_private_method('___a_'))
+        self.assertTrue(_is_private_method('___a_a'))
+        self.assertTrue(_is_private_method('___a_a_'))
+        self.assertTrue(_is_private_method('____a_a_'))
+
+        self.assertFalse(_is_private_method('_a'))
+        self.assertFalse(_is_private_method('_a_'))
+        self.assertFalse(_is_private_method('__a__'))
+        self.assertFalse(_is_private_method('__'))
+        self.assertFalse(_is_private_method('___'))
+        self.assertFalse(_is_private_method('____'))
+
+    def _assert_mangles_to(self, obj, name):
+        func = getattr(obj, name)
+        self.assertEqual(
+            _mangle_private_name(obj, func, func.__name__),
+            name
+        )
+
+    def test_private_name_mangling(self):
+        self._assert_mangles_to(
+            self.spider, '_TestSpider__parse_item_private')
+        if sys.version_info[0] >= 3:
+            self._assert_mangles_to(
+                self.spider, '_TestSpiderMixin__mixin_callback')
+
     def test_unserializable_callback1(self):
         r = Request("http://www.example.com", callback=lambda x: x)
         self.assertRaises(ValueError, request_to_dict, r)
@@ -78,7 +131,12 @@ class RequestSerializationTest(unittest.TestCase):
         self.assertRaises(ValueError, request_to_dict, r)
 
 
-class TestSpider(Spider):
+class TestSpiderMixin(object):
+    def __mixin_callback(self, response):
+        pass
+
+
+class TestSpider(Spider, TestSpiderMixin):
     name = 'test'
 
     def parse_item(self, response):
@@ -87,6 +145,9 @@ class TestSpider(Spider):
     def handle_error(self, failure):
         pass
 
+    def __parse_item_private(self, response):
+        pass
+
 
 class CustomRequest(Request):
     pass