Remove unused @lru_cache decorator

[Fizzadar]

Spotted this working on something else..

Contributed by Nick @ Beeper (@Fizzadar).

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

[DMRobertson]

Debian packaging unit tests failed. I wonder why?

 Traceback (most recent call last):
  File "/synapse/build/tests/util/caches/test_descriptors.py", line 459, in test_invalidate_cascade
    r = get_awaitable_result(obj.func1("k1", on_invalidate=top_invalidate))
  File "/synapse/build/tests/test_utils/__init__.py", line 40, in get_awaitable_result
    next(i)
  File "/synapse/build/tests/util/caches/test_descriptors.py", line 445, in func1
    return await self.func2(key, on_invalidate=cache_context.invalidate)
  File "/synapse/build/debian/matrix-synapse-py3/opt/venvs/matrix-synapse/lib/python3.10/site-packages/twisted/internet/defer.py", line 190, in maybeDeferred
    result = f(*args, **kwargs)
  File "/synapse/build/synapse/logging/context.py", line 753, in g
    return run_in_background(f, *args, **kwargs)
  File "/synapse/build/synapse/logging/context.py", line 814, in run_in_background
    res = defer.ensureDeferred(res)
  File "/synapse/build/debian/matrix-synapse-py3/opt/venvs/matrix-synapse/lib/python3.10/site-packages/twisted/internet/defer.py", line 1129, in ensureDeferred
    return Deferred.fromCoroutine(coro)
  File "/synapse/build/debian/matrix-synapse-py3/opt/venvs/matrix-synapse/lib/python3.10/site-packages/twisted/internet/defer.py", line 1105, in fromCoroutine
    return _cancellableInlineCallbacks(coro)
  File "/synapse/build/debian/matrix-synapse-py3/opt/venvs/matrix-synapse/lib/python3.10/site-packages/twisted/internet/defer.py", line 1815, in _cancellableInlineCallbacks
    _inlineCallbacks(None, gen, status)
  File "/synapse/build/debian/matrix-synapse-py3/opt/venvs/matrix-synapse/lib/python3.10/site-packages/twisted/internet/defer.py", line 1663, in _inlineCallbacks
    status.deferred.callback(getattr(e, "value", None))
  File "/synapse/build/debian/matrix-synapse-py3/opt/venvs/matrix-synapse/lib/python3.10/site-packages/twisted/internet/defer.py", line 660, in callback
    assert not isinstance(result, Deferred)

[DMRobertson]

Looks unused to me. We do use LruCache, but indirectly via @cached.

@matrix-org/synapse-core: any reason not to rip this out?

[clokep]

@matrix-org/synapse-core: any reason not to rip this out?

We can always bring it back via git. If it isn't used we should remove it.

[DMRobertson]

Consider me sniped. I have no idea what's going on though.

The test that's failing is

synapse/tests/util/caches/test_descriptors.py

Lines 439 to 462 in ed5ab47

	def test_invalidate_cascade(self):
	"""Invalidations should cascade up through cache contexts"""
	class Cls:
	@cached(cache_context=True)
	async def func1(self, key, cache_context):
	return await self.func2(key, on_invalidate=cache_context.invalidate)
	@cached(cache_context=True)
	async def func2(self, key, cache_context):
	return self.func3(key, on_invalidate=cache_context.invalidate)
	@cached(cache_context=True)
	def func3(self, key, cache_context):
	self.invalidate = cache_context.invalidate
	return 42
	obj = Cls()
	top_invalidate = mock.Mock()
	r = get_awaitable_result(obj.func1("k1", on_invalidate=top_invalidate))
	self.assertEqual(r, 42)
	obj.invalidate()
	top_invalidate.assert_called_once()

But I think it's not failing because we're removing code: it's failing just because we replaced this @lru_cache call with an @cached:

synapse/tests/util/caches/test_descriptors.py

Lines 451 to 454 in ed5ab47

	@cached(cache_context=True)
	def func3(self, key, cache_context):
	self.invalidate = cache_context.invalidate
	return 42

which seems like an utterly reasonable thing to do. The failure is just baffling.

[DMRobertson]

The failure is just baffling.

Further bafflement: if I make func3 an async function and await it in func2 then the test passes. Does this mean that an async @cached function cannot call synchronous @cached functions?

Edit: or maybe @cached isn't meant to decorate a synchronous function??? 🤯

[richvdh]

For the record, the one and only reference to @lru_cache was removed in #13078.

[clokep]

Edit: or maybe @cached isn't meant to decorate a synchronous function??? 🤯

I believe it can only be used with async functions.

[Fizzadar]

Thank you for picking this up @DMRobertson ❤️!

[DMRobertson]

Edit: or maybe @cached isn't meant to decorate a synchronous function??? exploding_head

I believe it can only be used with async functions.

Interesting (and not obvious at all). Searching for @cached.*\n *def I see only

synapse/synapse/storage/databases/main/roommember.py

Lines 1139 to 1141 in 0b7830e

	@cached(max_entries=10000)
	def _get_joined_hosts_cache(self, room_id: str) -> "_JoinedHostsCache":
	return _JoinedHostsCache()

outside of tests. (There are other sync functions in Synapse that are @cached, but they all raise NotImplementedError.

However there are many examples of synchronous @cached functions in tests.

[erikjohnston]

I am a bit confused why these changes are needed, given we seem to use maybeDeferred:

https://github.com/matrix-org/synapse/blob/19c0e55ef7742d67cff1cb6fb7c3e862b86ea788/synapse/util/caches/descriptors.py

[DMRobertson]

I would love to understand it more. Traceback for completeness.

I spent some time staring at this in a debugger and couldn't divine any insight.

==============================================================================
Error: 
Traceback (most recent call last):
  File "/home/runner/work/synapse/synapse/tests/util/caches/test_descriptors.py", line 459, in test_invalidate_cascade
    r = get_awaitable_result(obj.func1("k1", on_invalidate=top_invalidate))
  File "/home/runner/work/synapse/synapse/tests/test_utils/__init__.py", line 40, in get_awaitable_result
    next(i)
  File "/home/runner/work/synapse/synapse/tests/util/caches/test_descriptors.py", line 445, in func1
    return await self.func2(key, on_invalidate=cache_context.invalidate)
  File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.7/lib/python3.7/site-packages/twisted/internet/defer.py", line 151, in maybeDeferred
    result = f(*args, **kw)
  File "/home/runner/work/synapse/synapse/synapse/logging/context.py", line 753, in g
    return run_in_background(f, *args, **kwargs)
  File "/home/runner/work/synapse/synapse/synapse/logging/context.py", line 814, in run_in_background
    res = defer.ensureDeferred(res)
  File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.7/lib/python3.7/site-packages/twisted/internet/defer.py", line 911, in ensureDeferred
    return _cancellableInlineCallbacks(coro)
  File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.7/lib/python3.7/site-packages/twisted/internet/defer.py", line 1529, in _cancellableInlineCallbacks
    _inlineCallbacks(None, g, status)
  File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.7/lib/python3.7/site-packages/twisted/internet/defer.py", line 1421, in _inlineCallbacks
    status.deferred.callback(getattr(e, "value", None))
  File "/home/runner/.cache/pypoetry/virtualenvs/matrix-synapse-pswDeSvb-py3.7/lib/python3.7/site-packages/twisted/internet/defer.py", line 459, in callback
    assert not isinstance(result, Deferred)
builtins.AssertionError: 

tests.util.caches.test_descriptors.DescriptorTestCase.test_invalidate_cascade
-------------------------------------------------------------------------------