mirror of https://github.com/scrapy/scrapy.git
Fix to LocalCache with limit=0 still stores items rather than disabling the cache (#7663)
* Fix cell-var-from-loop in _send_catch_log_deferred Replace lambda capturing receiver by reference with a default argument to capture it by value, fixing a potential bug where all deferred callbacks could reference the last receiver in the loop instead of their respective receivers. Removes the pylint disable comment and TODO that were suppressing this issue. * chore: trigger CI rerun for mypy network error * Fix mypy error: pass receiver via addBoth args instead of lambda default * style: apply pre-commit ruff formatting * fix: LocalCache with limit=0 incorrectly stores items When limit=0 is passed to LocalCache (e.g. when DNSCACHE_ENABLED=False), the condition 'if self.limit' evaluates to False due to Python's truthiness rules, causing items to be stored despite the cache being disabled. This leads to an unbounded memory leak during long crawls when DNS caching is explicitly disabled. Fix changes the condition to 'if self.limit is not None' and adds an early return when limit=0 to correctly handle the disabled cache case. * test: add resolver-level regression tests for DNSCACHE_ENABLED=False Add two regression tests that verify DNS results are not stored in dnscache when DNSCACHE_ENABLED=False (cache_size=0): - test_caching_hostname_resolver_dnscache_disabled_rejects_storage: verifies _CachingResolutionReceiver does not write to dnscache when CachingHostnameResolver is initialized with cache_size=0 - test_caching_threaded_resolver_dnscache_disabled_rejects_storage: verifies dnscache rejects storage at the LocalCache level when limit=0 * test: drop misleading threaded resolver test it was writing directly to dnscache, not actually going through CachingThreadedResolver at all. the threaded resolver already has its own if dnscache.limit: guard so the bug doesnt affect it anyway. keeping only the hostname resolver test which covers the actual bug path through _CachingResolutionReceiver * test: clean up comments in resolver test * test: remove unnecessary comment
This commit is contained in:
parent
4b2b56f384
commit
6591cb756c
|
|
@ -152,7 +152,9 @@ class LocalCache(OrderedDict[_KT, _VT]):
|
|||
self.limit: int | None = limit
|
||||
|
||||
def __setitem__(self, key: _KT, value: _VT) -> None:
|
||||
if self.limit:
|
||||
if self.limit is not None:
|
||||
if self.limit == 0:
|
||||
return
|
||||
while len(self) >= self.limit:
|
||||
self.popitem(last=False)
|
||||
super().__setitem__(key, value)
|
||||
|
|
|
|||
|
|
@ -53,3 +53,21 @@ def test_caching_hostname_resolver_no_addresses_not_cached():
|
|||
resolver.resolveHostName(Mock(), "example.com")
|
||||
|
||||
assert "example.com" not in dnscache
|
||||
|
||||
|
||||
def test_caching_hostname_resolver_dnscache_disabled_rejects_storage():
|
||||
|
||||
def fake_resolve(receiver, *_):
|
||||
receiver.resolutionBegan(Mock())
|
||||
receiver.addressResolved(Mock())
|
||||
receiver.resolutionComplete()
|
||||
return receiver
|
||||
|
||||
reactor = Mock()
|
||||
reactor.nameResolver.resolveHostName.side_effect = fake_resolve
|
||||
|
||||
resolver = CachingHostnameResolver(reactor, cache_size=0)
|
||||
resolver.resolveHostName(Mock(), "example.com")
|
||||
|
||||
assert "example.com" not in dnscache
|
||||
assert len(dnscache) == 0
|
||||
|
|
|
|||
|
|
@ -309,6 +309,16 @@ class TestLocalCache:
|
|||
assert str(x) in cache
|
||||
assert cache[str(x)] == x
|
||||
|
||||
def test_cache_with_zero_limit(self):
|
||||
cache = LocalCache(limit=0)
|
||||
cache["a"] = 1
|
||||
cache["b"] = 2
|
||||
cache["c"] = 3
|
||||
assert len(cache) == 0
|
||||
assert "a" not in cache
|
||||
assert "b" not in cache
|
||||
assert "c" not in cache
|
||||
|
||||
|
||||
class TestLocalWeakReferencedCache:
|
||||
def test_cache_with_limit(self):
|
||||
|
|
|
|||
Loading…
Reference in New Issue