diff --git a/scrapy/core/scraper.py b/scrapy/core/scraper.py index b3d585cce..99114d3bb 100644 --- a/scrapy/core/scraper.py +++ b/scrapy/core/scraper.py @@ -9,7 +9,7 @@ from twisted.internet import defer from scrapy.utils.defer import defer_result, defer_succeed, parallel, iter_errback from scrapy.utils.spider import iterate_spider_output -from scrapy.utils.misc import load_object +from scrapy.utils.misc import load_object, warn_on_generator_with_return_value from scrapy.utils.log import logformatter_adapter, failure_to_exc_info from scrapy.exceptions import CloseSpider, DropItem, IgnoreRequest from scrapy import signals @@ -18,6 +18,7 @@ from scrapy.item import BaseItem from scrapy.core.spidermw import SpiderMiddlewareManager from scrapy.utils.request import referer_str + logger = logging.getLogger(__name__) @@ -99,11 +100,13 @@ class Scraper(object): def enqueue_scrape(self, response, request, spider): slot = self.slot dfd = slot.add_response_request(response, request) + def finish_scraping(_): slot.finish_response(response, request) self._check_if_closing(spider, slot) self._scrape_next(spider, slot) return _ + dfd.addBoth(finish_scraping) dfd.addErrback( lambda f: logger.error('Scraper bug processing %(request)s', @@ -123,7 +126,7 @@ class Scraper(object): callback/errback""" assert isinstance(response, (Response, Failure)) - dfd = self._scrape2(response, request, spider) # returns spiders processed output + dfd = self._scrape2(response, request, spider) # returns spider's processed output dfd.addErrback(self.handle_spider_error, request, response, spider) dfd.addCallback(self.handle_spider_output, request, response, spider) return dfd @@ -142,7 +145,10 @@ class Scraper(object): def call_spider(self, result, request, spider): result.request = request dfd = defer_result(result) - dfd.addCallbacks(callback=request.callback or spider.parse, + callback = request.callback or spider.parse + warn_on_generator_with_return_value(spider, callback) + warn_on_generator_with_return_value(spider, request.errback) + dfd.addCallbacks(callback=callback, errback=request.errback, callbackKeywords=request.cb_kwargs) return dfd.addCallback(iterate_spider_output) @@ -172,8 +178,8 @@ class Scraper(object): if not result: return defer_succeed(None) it = iter_errback(result, self.handle_spider_error, request, response, spider) - dfd = parallel(it, self.concurrent_items, - self._process_spidermw_output, request, response, spider) + dfd = parallel(it, self.concurrent_items, self._process_spidermw_output, + request, response, spider) return dfd def _process_spidermw_output(self, output, request, response, spider): @@ -200,8 +206,7 @@ class Scraper(object): """Log and silence errors that come from the engine (typically download errors that got propagated thru here) """ - if (isinstance(download_failure, Failure) and - not download_failure.check(IgnoreRequest)): + if isinstance(download_failure, Failure) and not download_failure.check(IgnoreRequest): if download_failure.frames: logger.error('Error downloading %(request)s', {'request': request}, diff --git a/scrapy/utils/datatypes.py b/scrapy/utils/datatypes.py index ffd1537c3..a52bbc70e 100644 --- a/scrapy/utils/datatypes.py +++ b/scrapy/utils/datatypes.py @@ -8,6 +8,7 @@ This module must not depend on any module outside the Standard Library. import collections import copy import warnings +import weakref from collections.abc import Mapping from scrapy.exceptions import ScrapyDeprecationWarning @@ -240,7 +241,6 @@ class LocalCache(collections.OrderedDict): """Dictionary with a finite number of keys. Older items expires first. - """ def __init__(self, limit=None): @@ -254,6 +254,35 @@ class LocalCache(collections.OrderedDict): super(LocalCache, self).__setitem__(key, value) +class LocalWeakReferencedCache(weakref.WeakKeyDictionary): + """ + A weakref.WeakKeyDictionary implementation that uses LocalCache as its + underlying data structure, making it ordered and capable of being size-limited. + + Useful for memoization, while avoiding keeping received + arguments in memory only because of the cached references. + + Note: like LocalCache and unlike weakref.WeakKeyDictionary, + it cannot be instantiated with an initial dictionary. + """ + + def __init__(self, limit=None): + super(LocalWeakReferencedCache, self).__init__() + self.data = LocalCache(limit=limit) + + def __setitem__(self, key, value): + try: + super(LocalWeakReferencedCache, self).__setitem__(key, value) + except TypeError: + pass # key is not weak-referenceable, skip caching + + def __getitem__(self, key): + try: + return super(LocalWeakReferencedCache, self).__getitem__(key) + except TypeError: + return None # key is not weak-referenceable, it's not cached + + class SequenceExclude(object): """Object to test if an item is NOT within some sequence.""" diff --git a/scrapy/utils/misc.py b/scrapy/utils/misc.py index 9955fb1e7..cb0ee5af3 100644 --- a/scrapy/utils/misc.py +++ b/scrapy/utils/misc.py @@ -1,13 +1,18 @@ """Helper functions which don't fit anywhere else""" +import ast +import inspect import os import re import hashlib +import warnings from contextlib import contextmanager from importlib import import_module from pkgutil import iter_modules +from textwrap import dedent from w3lib.html import replace_entities +from scrapy.utils.datatypes import LocalWeakReferencedCache from scrapy.utils.python import flatten, to_unicode from scrapy.item import BaseItem @@ -161,3 +166,44 @@ def set_environ(**kwargs): del os.environ[k] else: os.environ[k] = v + + +_generator_callbacks_cache = LocalWeakReferencedCache(limit=128) + + +def is_generator_with_return_value(callable): + """ + Returns True if a callable is a generator function which includes a + 'return' statement with a value different than None, False otherwise + """ + if callable in _generator_callbacks_cache: + return _generator_callbacks_cache[callable] + + def returns_none(return_node): + value = return_node.value + return value is None or isinstance(value, ast.NameConstant) and value.value is None + + if inspect.isgeneratorfunction(callable): + tree = ast.parse(dedent(inspect.getsource(callable))) + for node in ast.walk(tree): + if isinstance(node, ast.Return) and not returns_none(node): + _generator_callbacks_cache[callable] = True + return _generator_callbacks_cache[callable] + + _generator_callbacks_cache[callable] = False + return _generator_callbacks_cache[callable] + + +def warn_on_generator_with_return_value(spider, callable): + """ + Logs a warning if a callable is a generator function and includes + a 'return' statement with a value different than None + """ + if is_generator_with_return_value(callable): + warnings.warn( + 'The "{}.{}" method is a generator and includes a "return" statement with a ' + 'value different than None. This could lead to unexpected behaviour. Please see ' + 'https://docs.python.org/3/reference/simple_stmts.html#the-return-statement ' + 'for details about the semantics of the "return" statement within generators' + .format(spider.__class__.__name__, callable.__name__), stacklevel=2, + ) diff --git a/tests/test_utils_datatypes.py b/tests/test_utils_datatypes.py index 38a25778e..e5aa56eb9 100644 --- a/tests/test_utils_datatypes.py +++ b/tests/test_utils_datatypes.py @@ -2,7 +2,9 @@ import copy import unittest from collections.abc import Mapping, MutableMapping -from scrapy.utils.datatypes import CaselessDict, LocalCache, SequenceExclude +from scrapy.http import Request +from scrapy.utils.datatypes import CaselessDict, LocalCache, LocalWeakReferencedCache, SequenceExclude +from scrapy.utils.python import garbage_collect __doctests__ = ['scrapy.utils.datatypes'] @@ -255,5 +257,67 @@ class LocalCacheTest(unittest.TestCase): self.assertEqual(cache[str(x)], x) +class LocalWeakReferencedCacheTest(unittest.TestCase): + + def test_cache_with_limit(self): + cache = LocalWeakReferencedCache(limit=2) + r1 = Request('https://example.org') + r2 = Request('https://example.com') + r3 = Request('https://example.net') + cache[r1] = 1 + cache[r2] = 2 + cache[r3] = 3 + self.assertEqual(len(cache), 2) + self.assertNotIn(r1, cache) + self.assertIn(r2, cache) + self.assertIn(r3, cache) + self.assertEqual(cache[r2], 2) + self.assertEqual(cache[r3], 3) + del r2 + + # PyPy takes longer to collect dead references + garbage_collect() + + self.assertEqual(len(cache), 1) + + def test_cache_non_weak_referenceable_objects(self): + cache = LocalWeakReferencedCache() + k1 = None + k2 = 1 + k3 = [1, 2, 3] + cache[k1] = 1 + cache[k2] = 2 + cache[k3] = 3 + self.assertNotIn(k1, cache) + self.assertNotIn(k2, cache) + self.assertNotIn(k3, cache) + self.assertEqual(len(cache), 0) + + def test_cache_without_limit(self): + max = 10**4 + cache = LocalWeakReferencedCache() + refs = [] + for x in range(max): + refs.append(Request('https://example.org/{}'.format(x))) + cache[refs[-1]] = x + self.assertEqual(len(cache), max) + for i, r in enumerate(refs): + self.assertIn(r, cache) + self.assertEqual(cache[r], i) + del r # delete reference to the last object in the list + + # delete half of the objects, make sure that is reflected in the cache + for _ in range(max // 2): + refs.pop() + + # PyPy takes longer to collect dead references + garbage_collect() + + self.assertEqual(len(cache), max // 2) + for i, r in enumerate(refs): + self.assertIn(r, cache) + self.assertEqual(cache[r], i) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_utils_misc/test_return_with_argument_inside_generator.py b/tests/test_utils_misc/test_return_with_argument_inside_generator.py new file mode 100644 index 000000000..bdbec1beb --- /dev/null +++ b/tests/test_utils_misc/test_return_with_argument_inside_generator.py @@ -0,0 +1,37 @@ +import unittest + +from scrapy.utils.misc import is_generator_with_return_value + + +class UtilsMiscPy3TestCase(unittest.TestCase): + + def test_generators_with_return_statements(self): + def f(): + yield 1 + return 2 + + def g(): + yield 1 + return 'asdf' + + def h(): + yield 1 + return None + + def i(): + yield 1 + return + + def j(): + yield 1 + + def k(): + yield 1 + yield from g() + + assert is_generator_with_return_value(f) + assert is_generator_with_return_value(g) + assert not is_generator_with_return_value(h) + assert not is_generator_with_return_value(i) + assert not is_generator_with_return_value(j) + assert not is_generator_with_return_value(k) # not recursive