From 95e9399db2646b2109ca5ccd56739458ef65236f Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Mon, 22 Feb 2021 15:00:46 +0100 Subject: [PATCH 1/8] add tests for response_cache.py make first argument to ResponseCache synapse.util.Clock instead of HomeServer, to make more accessible to tests + as a result, hs.get_clock() needs to be added in all places + and in some places, HomeServer type annotation should be added --- synapse/appservice/api.py | 2 +- synapse/federation/federation_server.py | 8 +- synapse/handlers/initial_sync.py | 2 +- synapse/handlers/room.py | 2 +- synapse/handlers/room_list.py | 4 +- synapse/handlers/sync.py | 2 +- synapse/replication/http/_base.py | 9 +- synapse/util/caches/response_cache.py | 10 +- tests/util/caches/test_responsecache.py | 269 ++++++++++++++++++++++++ 9 files changed, 289 insertions(+), 19 deletions(-) create mode 100644 tests/util/caches/test_responsecache.py diff --git a/synapse/appservice/api.py b/synapse/appservice/api.py index 93c2aabcca6c..9d3bbe3b8b05 100644 --- a/synapse/appservice/api.py +++ b/synapse/appservice/api.py @@ -90,7 +90,7 @@ def __init__(self, hs): self.clock = hs.get_clock() self.protocol_meta_cache = ResponseCache( - hs, "as_protocol_meta", timeout_ms=HOUR_IN_MS + hs.get_clock(), "as_protocol_meta", timeout_ms=HOUR_IN_MS ) # type: ResponseCache[Tuple[str, str]] async def query_user(self, service, user_id): diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 2f832b47f65c..552b3f1e5635 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -99,7 +99,7 @@ class FederationServer(FederationBase): - def __init__(self, hs): + def __init__(self, hs: "HomeServer"): super().__init__(hs) self.auth = hs.get_auth() @@ -119,7 +119,7 @@ def __init__(self, hs): # We cache results for transaction with the same ID self._transaction_resp_cache = ResponseCache( - hs, "fed_txn_handler", timeout_ms=30000 + hs.get_clock(), "fed_txn_handler", timeout_ms=30000 ) # type: ResponseCache[Tuple[str, str]] self.transaction_actions = TransactionActions(self.store) @@ -129,10 +129,10 @@ def __init__(self, hs): # We cache responses to state queries, as they take a while and often # come in waves. self._state_resp_cache = ResponseCache( - hs, "state_resp", timeout_ms=30000 + hs.get_clock(), "state_resp", timeout_ms=30000 ) # type: ResponseCache[Tuple[str, str]] self._state_ids_resp_cache = ResponseCache( - hs, "state_ids_resp", timeout_ms=30000 + hs.get_clock(), "state_ids_resp", timeout_ms=30000 ) # type: ResponseCache[Tuple[str, str]] self._federation_metrics_domains = ( diff --git a/synapse/handlers/initial_sync.py b/synapse/handlers/initial_sync.py index 71a507667296..13f8152283f2 100644 --- a/synapse/handlers/initial_sync.py +++ b/synapse/handlers/initial_sync.py @@ -48,7 +48,7 @@ def __init__(self, hs: "HomeServer"): self.clock = hs.get_clock() self.validator = EventValidator() self.snapshot_cache = ResponseCache( - hs, "initial_sync_cache" + hs.get_clock(), "initial_sync_cache" ) # type: ResponseCache[Tuple[str, Optional[StreamToken], Optional[StreamToken], str, Optional[int], bool, bool]] self._event_serializer = hs.get_event_client_serializer() self.storage = hs.get_storage() diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index a488df10d678..4b3d0d72e387 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -121,7 +121,7 @@ def __init__(self, hs: "HomeServer"): # succession, only process the first attempt and return its result to # subsequent requests self._upgrade_response_cache = ResponseCache( - hs, "room_upgrade", timeout_ms=FIVE_MINUTES_IN_MS + hs.get_clock(), "room_upgrade", timeout_ms=FIVE_MINUTES_IN_MS ) # type: ResponseCache[Tuple[str, str]] self._server_notices_mxid = hs.config.server_notices_mxid diff --git a/synapse/handlers/room_list.py b/synapse/handlers/room_list.py index 14f14db4492b..8bfc46c6545e 100644 --- a/synapse/handlers/room_list.py +++ b/synapse/handlers/room_list.py @@ -44,10 +44,10 @@ def __init__(self, hs: "HomeServer"): super().__init__(hs) self.enable_room_list_search = hs.config.enable_room_list_search self.response_cache = ResponseCache( - hs, "room_list" + hs.get_clock(), "room_list" ) # type: ResponseCache[Tuple[Optional[int], Optional[str], ThirdPartyInstanceID]] self.remote_response_cache = ResponseCache( - hs, "remote_room_list", timeout_ms=30 * 1000 + hs.get_clock(), "remote_room_list", timeout_ms=30 * 1000 ) # type: ResponseCache[Tuple[str, Optional[int], Optional[str], bool, Optional[str]]] async def get_local_public_room_list( diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index 4e8ed7b33f62..f50257cd5736 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -244,7 +244,7 @@ def __init__(self, hs: "HomeServer"): self.event_sources = hs.get_event_sources() self.clock = hs.get_clock() self.response_cache = ResponseCache( - hs, "sync" + hs.get_clock(), "sync" ) # type: ResponseCache[Tuple[Any, ...]] self.state = hs.get_state_handler() self.auth = hs.get_auth() diff --git a/synapse/replication/http/_base.py b/synapse/replication/http/_base.py index 8a3f113e7640..b7aa0c280fe5 100644 --- a/synapse/replication/http/_base.py +++ b/synapse/replication/http/_base.py @@ -18,7 +18,7 @@ import re import urllib from inspect import signature -from typing import Dict, List, Tuple +from typing import TYPE_CHECKING, Dict, List, Tuple from prometheus_client import Counter, Gauge @@ -28,6 +28,9 @@ from synapse.util.caches.response_cache import ResponseCache from synapse.util.stringutils import random_string +if TYPE_CHECKING: + from synapse.server import HomeServer + logger = logging.getLogger(__name__) _pending_outgoing_requests = Gauge( @@ -88,10 +91,10 @@ class ReplicationEndpoint(metaclass=abc.ABCMeta): CACHE = True RETRY_ON_TIMEOUT = True - def __init__(self, hs): + def __init__(self, hs: "HomeServer"): if self.CACHE: self.response_cache = ResponseCache( - hs, "repl." + self.NAME, timeout_ms=30 * 60 * 1000 + hs.get_clock(), "repl." + self.NAME, timeout_ms=30 * 60 * 1000 ) # type: ResponseCache[str] # We reserve `instance_name` as a parameter to sending requests, so we diff --git a/synapse/util/caches/response_cache.py b/synapse/util/caches/response_cache.py index 32228f42ee59..816b3714be7e 100644 --- a/synapse/util/caches/response_cache.py +++ b/synapse/util/caches/response_cache.py @@ -13,17 +13,15 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging -from typing import TYPE_CHECKING, Any, Callable, Dict, Generic, Optional, TypeVar +from typing import Any, Callable, Dict, Generic, Optional, Set, TypeVar from twisted.internet import defer from synapse.logging.context import make_deferred_yieldable, run_in_background +from synapse.util import Clock from synapse.util.async_helpers import ObservableDeferred from synapse.util.caches import register_cache -if TYPE_CHECKING: - from synapse.app.homeserver import HomeServer - logger = logging.getLogger(__name__) T = TypeVar("T") @@ -37,11 +35,11 @@ class ResponseCache(Generic[T]): used rather than trying to compute a new response. """ - def __init__(self, hs: "HomeServer", name: str, timeout_ms: float = 0): + def __init__(self, clock: Clock, name: str, timeout_ms: float = 0): # Requests that haven't finished yet. self.pending_result_cache = {} # type: Dict[T, ObservableDeferred] - self.clock = hs.get_clock() + self.clock = clock self.timeout_sec = timeout_ms / 1000.0 self._name = name diff --git a/tests/util/caches/test_responsecache.py b/tests/util/caches/test_responsecache.py new file mode 100644 index 000000000000..a55ef4b757bd --- /dev/null +++ b/tests/util/caches/test_responsecache.py @@ -0,0 +1,269 @@ +# Copyright 2021 Vector Creations Ltd +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from twisted.internet.task import Clock + +from synapse.util import Clock as SynapseClock +from synapse.util.caches.response_cache import ResponseCache + +from tests.unittest import TestCase + +# few notes about test naming here: +# 'wait': denotes tests that have an element of "waiting" before its wrapped result becomes available +# 'expire': denotes tests that test expiry after assured existence + + +class DeferredCacheTestCase(TestCase): + def setUp(self): + self.reactor = Clock() + self.synapse_clock = SynapseClock(self.reactor) + + @staticmethod + async def instant_return(o: str) -> str: + return o + + async def delayed_return(self, o: str) -> str: + await self.synapse_clock.sleep(1) + return o + + def test_cache_hit(self): + cache = ResponseCache(self.synapse_clock, "keeping_cache", timeout_ms=9001) + + expected_result = "howdy" + + wrap_d = cache.wrap(0, self.instant_return, expected_result) + + self.assertEqual( + expected_result, + self.successResultOf(wrap_d), + "initial wrap result should be the same", + ) + self.assertEqual( + expected_result, + self.successResultOf(cache.get(0)), + "cache should have the result", + ) + + def test_cache_miss(self): + cache = ResponseCache(self.synapse_clock, "trashing_cache", timeout_ms=0) + + expected_result = "howdy" + + wrap_d = cache.wrap(0, self.instant_return, expected_result) + + self.assertEqual( + expected_result, + self.successResultOf(wrap_d), + "initial wrap result should be the same", + ) + self.assertIsNone(cache.get(0), "cache should not have the result now") + + def test_cache_expire(self): + cache = ResponseCache(self.synapse_clock, "short_cache", timeout_ms=1000) + + expected_result = "howdy" + + wrap_d = cache.wrap(0, self.instant_return, expected_result) + + self.assertEqual(expected_result, self.successResultOf(wrap_d)) + self.assertEqual( + expected_result, + self.successResultOf(cache.get(0)), + "cache should still have the result", + ) + + # cache eviction timer is handled + self.reactor.pump((2,)) + + self.assertIsNone(cache.get(0), "cache should not have the result now") + + def test_cache_wait_hit(self): + cache = ResponseCache(self.synapse_clock, "neutral_cache") + + expected_result = "howdy" + + wrap_d = cache.wrap(0, self.delayed_return, expected_result) + self.assertNoResult(wrap_d) + + # function wakes up, returns result + self.reactor.pump((2,)) + + self.assertEqual(expected_result, self.successResultOf(wrap_d)) + + def test_cache_wait_expire(self): + cache = ResponseCache(self.synapse_clock, "short_cache", timeout_ms=3000) + + expected_result = "howdy" + + wrap_d = cache.wrap(0, self.delayed_return, expected_result) + self.assertNoResult(wrap_d) + + # stop at 1 second to callback cache eviction callLater at that time, then another to set time at 2 + self.reactor.pump((1, 1)) + + self.assertEqual(expected_result, self.successResultOf(wrap_d)) + self.assertEqual( + expected_result, + self.successResultOf(cache.get(0)), + "cache should still have the result", + ) + + # (1 + 1 + 2) < 3.0, cache eviction timer is handled + self.reactor.pump((2,)) + + self.assertIsNone(cache.get(0), "cache should not have the result now") + + +class ConditionalDeferredCacheTestCase(DeferredCacheTestCase): + def test_one_false(self): + cache = ResponseCache(self.synapse_clock, "keeping_cache", timeout_ms=9001) + + texan_greeting = "howdy" + english_greeting = "greetings" + + def is_english_greeting(greeting) -> bool: + return greeting == english_greeting + + wrap_d = cache.wrap_conditional( + 0, is_english_greeting, self.instant_return, texan_greeting + ) + + self.assertEqual(texan_greeting, self.successResultOf(wrap_d)) + self.assertIsNone(cache.get(0), "cache should not have the result") + + def test_one_true(self): + cache = ResponseCache(self.synapse_clock, "keeping_cache", timeout_ms=9001) + + texan_greeting = "howdy" + + def is_texan_greeting(greeting) -> bool: + return greeting == texan_greeting + + wrap_d = cache.wrap_conditional( + 0, is_texan_greeting, self.instant_return, texan_greeting + ) + + self.assertEqual(texan_greeting, self.successResultOf(wrap_d)) + self.assertEqual( + texan_greeting, + self.successResultOf(cache.get(0)), + "cache should have the result", + ) + + def test_one_false_with_empty(self): + cache = ResponseCache(self.synapse_clock, "keeping_cache", timeout_ms=9001) + + texan_greeting = "howdy" + english_greeting = "greetings" + + def is_english_greeting(greeting) -> bool: + return greeting == english_greeting + + wrap_d = cache.wrap_conditional( + 0, is_english_greeting, self.delayed_return, texan_greeting + ) + wrap_empty = cache.wrap(0, self.delayed_return, texan_greeting) + + self.reactor.pump((1,)) + + self.assertEqual(texan_greeting, self.successResultOf(wrap_d)) + self.assertEqual(texan_greeting, self.successResultOf(wrap_empty)) + self.assertIsNone(cache.get(0), "cache should not have the result") + + def test_one_true_with_empty(self): + cache = ResponseCache(self.synapse_clock, "keeping_cache", timeout_ms=9001) + + texan_greeting = "howdy" + + def is_texan_greeting(greeting) -> bool: + return greeting == texan_greeting + + wrap_d = cache.wrap_conditional( + 0, is_texan_greeting, self.delayed_return, texan_greeting + ) + wrap_empty = cache.wrap(0, self.delayed_return, texan_greeting) + + self.reactor.pump((1,)) + + self.assertEqual(texan_greeting, self.successResultOf(wrap_d)) + self.assertEqual(texan_greeting, self.successResultOf(wrap_empty)) + self.assertEqual( + texan_greeting, + self.successResultOf(cache.get(0)), + "cache should have the result", + ) + + def test_multiple_mixed(self): + cache = ResponseCache(self.synapse_clock, "keeping_cache", timeout_ms=9001) + + texan_greeting = "howdy" + english_greeting = "greetings" + + def is_english_greeting(greeting) -> bool: + return greeting == english_greeting + + def is_texan_greeting(greeting) -> bool: + return greeting == texan_greeting + + negative_wrap = cache.wrap_conditional( + 0, is_english_greeting, self.delayed_return, texan_greeting + ) + + positive_wraps = { + cache.wrap_conditional( + 0, is_texan_greeting, self.delayed_return, texan_greeting + ) + for _ in range(5) + } + + self.reactor.pump((1,)) + + for wrap in positive_wraps | {negative_wrap}: + self.assertEqual( + texan_greeting, + self.successResultOf(wrap), + "wrap deferred {!r} should have {!r}".format(wrap, texan_greeting), + ) + + self.assertIsNone(cache.get(0), "cache should not have the result") + + def test_multiple_true(self): + cache = ResponseCache(self.synapse_clock, "keeping_cache", timeout_ms=9001) + + texan_greeting = "howdy" + + def is_texan_greeting(greeting) -> bool: + return greeting == texan_greeting + + positive_wraps = { + cache.wrap_conditional( + 0, is_texan_greeting, self.delayed_return, texan_greeting + ) + for _ in range(6) + } + + self.reactor.pump((1,)) + + for wrap in positive_wraps: + self.assertEqual( + texan_greeting, + self.successResultOf(wrap), + "wrap deferred {!r} should have {!r}".format(wrap, texan_greeting), + ) + + self.assertEqual( + texan_greeting, + self.successResultOf(cache.get(0)), + "cache should have the result", + ) From aee352a3ee94016d0e86727eb43b226784ad77ca Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Mon, 22 Feb 2021 15:01:11 +0100 Subject: [PATCH 2/8] fix type technicality in federation_server.py --- synapse/federation/federation_server.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 552b3f1e5635..66eb3828105d 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -22,6 +22,7 @@ Awaitable, Callable, Dict, + Iterable, List, Optional, Tuple, @@ -454,6 +455,7 @@ async def _on_state_ids_request_compute(self, room_id, event_id): async def _on_context_state_request_compute( self, room_id: str, event_id: str ) -> Dict[str, list]: + pdus = [] # type: Iterable[EventBase] if event_id: pdus = await self.handler.get_state_for_pdu(room_id, event_id) else: From f4549e3f9fa589ef50a239dda5a7876c250021a2 Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Mon, 22 Feb 2021 15:27:41 +0100 Subject: [PATCH 3/8] news --- changelog.d/9458.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/9458.misc diff --git a/changelog.d/9458.misc b/changelog.d/9458.misc new file mode 100644 index 000000000000..8ceeed1352e5 --- /dev/null +++ b/changelog.d/9458.misc @@ -0,0 +1 @@ +Add tests to ResponseCache. \ No newline at end of file From 4140f74bedc1ee606a8b382fa1715fca46277c3b Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Fri, 26 Feb 2021 21:44:23 +0100 Subject: [PATCH 4/8] alter type comment --- synapse/federation/federation_server.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 66eb3828105d..97916f128a9f 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -455,9 +455,10 @@ async def _on_state_ids_request_compute(self, room_id, event_id): async def _on_context_state_request_compute( self, room_id: str, event_id: str ) -> Dict[str, list]: - pdus = [] # type: Iterable[EventBase] if event_id: - pdus = await self.handler.get_state_for_pdu(room_id, event_id) + pdus = await self.handler.get_state_for_pdu( + room_id, event_id + ) # type: Iterable[EventBase] else: pdus = (await self.state.get_current_state(room_id)).values() From 86e0e06b05524d5e87fbd62855629ab4bd6cb2ed Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Wed, 3 Mar 2021 14:41:55 +0100 Subject: [PATCH 5/8] remove ConditionalDeferredCacheTestCase --- tests/util/caches/test_responsecache.py | 144 ------------------------ 1 file changed, 144 deletions(-) diff --git a/tests/util/caches/test_responsecache.py b/tests/util/caches/test_responsecache.py index a55ef4b757bd..39a08a77f662 100644 --- a/tests/util/caches/test_responsecache.py +++ b/tests/util/caches/test_responsecache.py @@ -123,147 +123,3 @@ def test_cache_wait_expire(self): self.reactor.pump((2,)) self.assertIsNone(cache.get(0), "cache should not have the result now") - - -class ConditionalDeferredCacheTestCase(DeferredCacheTestCase): - def test_one_false(self): - cache = ResponseCache(self.synapse_clock, "keeping_cache", timeout_ms=9001) - - texan_greeting = "howdy" - english_greeting = "greetings" - - def is_english_greeting(greeting) -> bool: - return greeting == english_greeting - - wrap_d = cache.wrap_conditional( - 0, is_english_greeting, self.instant_return, texan_greeting - ) - - self.assertEqual(texan_greeting, self.successResultOf(wrap_d)) - self.assertIsNone(cache.get(0), "cache should not have the result") - - def test_one_true(self): - cache = ResponseCache(self.synapse_clock, "keeping_cache", timeout_ms=9001) - - texan_greeting = "howdy" - - def is_texan_greeting(greeting) -> bool: - return greeting == texan_greeting - - wrap_d = cache.wrap_conditional( - 0, is_texan_greeting, self.instant_return, texan_greeting - ) - - self.assertEqual(texan_greeting, self.successResultOf(wrap_d)) - self.assertEqual( - texan_greeting, - self.successResultOf(cache.get(0)), - "cache should have the result", - ) - - def test_one_false_with_empty(self): - cache = ResponseCache(self.synapse_clock, "keeping_cache", timeout_ms=9001) - - texan_greeting = "howdy" - english_greeting = "greetings" - - def is_english_greeting(greeting) -> bool: - return greeting == english_greeting - - wrap_d = cache.wrap_conditional( - 0, is_english_greeting, self.delayed_return, texan_greeting - ) - wrap_empty = cache.wrap(0, self.delayed_return, texan_greeting) - - self.reactor.pump((1,)) - - self.assertEqual(texan_greeting, self.successResultOf(wrap_d)) - self.assertEqual(texan_greeting, self.successResultOf(wrap_empty)) - self.assertIsNone(cache.get(0), "cache should not have the result") - - def test_one_true_with_empty(self): - cache = ResponseCache(self.synapse_clock, "keeping_cache", timeout_ms=9001) - - texan_greeting = "howdy" - - def is_texan_greeting(greeting) -> bool: - return greeting == texan_greeting - - wrap_d = cache.wrap_conditional( - 0, is_texan_greeting, self.delayed_return, texan_greeting - ) - wrap_empty = cache.wrap(0, self.delayed_return, texan_greeting) - - self.reactor.pump((1,)) - - self.assertEqual(texan_greeting, self.successResultOf(wrap_d)) - self.assertEqual(texan_greeting, self.successResultOf(wrap_empty)) - self.assertEqual( - texan_greeting, - self.successResultOf(cache.get(0)), - "cache should have the result", - ) - - def test_multiple_mixed(self): - cache = ResponseCache(self.synapse_clock, "keeping_cache", timeout_ms=9001) - - texan_greeting = "howdy" - english_greeting = "greetings" - - def is_english_greeting(greeting) -> bool: - return greeting == english_greeting - - def is_texan_greeting(greeting) -> bool: - return greeting == texan_greeting - - negative_wrap = cache.wrap_conditional( - 0, is_english_greeting, self.delayed_return, texan_greeting - ) - - positive_wraps = { - cache.wrap_conditional( - 0, is_texan_greeting, self.delayed_return, texan_greeting - ) - for _ in range(5) - } - - self.reactor.pump((1,)) - - for wrap in positive_wraps | {negative_wrap}: - self.assertEqual( - texan_greeting, - self.successResultOf(wrap), - "wrap deferred {!r} should have {!r}".format(wrap, texan_greeting), - ) - - self.assertIsNone(cache.get(0), "cache should not have the result") - - def test_multiple_true(self): - cache = ResponseCache(self.synapse_clock, "keeping_cache", timeout_ms=9001) - - texan_greeting = "howdy" - - def is_texan_greeting(greeting) -> bool: - return greeting == texan_greeting - - positive_wraps = { - cache.wrap_conditional( - 0, is_texan_greeting, self.delayed_return, texan_greeting - ) - for _ in range(6) - } - - self.reactor.pump((1,)) - - for wrap in positive_wraps: - self.assertEqual( - texan_greeting, - self.successResultOf(wrap), - "wrap deferred {!r} should have {!r}".format(wrap, texan_greeting), - ) - - self.assertEqual( - texan_greeting, - self.successResultOf(cache.get(0)), - "cache should have the result", - ) From b8edfef783a27f39002533e88459e5396cc3c3b3 Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Wed, 3 Mar 2021 14:49:50 +0100 Subject: [PATCH 6/8] remove crack in time :) --- synapse/util/caches/response_cache.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/util/caches/response_cache.py b/synapse/util/caches/response_cache.py index 816b3714be7e..46ea8e09644c 100644 --- a/synapse/util/caches/response_cache.py +++ b/synapse/util/caches/response_cache.py @@ -13,7 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging -from typing import Any, Callable, Dict, Generic, Optional, Set, TypeVar +from typing import Any, Callable, Dict, Generic, Optional, TypeVar from twisted.internet import defer From e85409e3eb2728fe707bf9cb46c9db24afecbfd5 Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Mon, 8 Mar 2021 18:23:29 +0100 Subject: [PATCH 7/8] apply review feedback - use tests.server.get_clock() - alter copyright (to matrix.org foundation) - move testcase function naming convention description to docstring - alter Cache instantiation a but to now happen in separate function (more readable) --- tests/util/caches/test_responsecache.py | 38 ++++++++++++++----------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/tests/util/caches/test_responsecache.py b/tests/util/caches/test_responsecache.py index 39a08a77f662..a429a62b53ae 100644 --- a/tests/util/caches/test_responsecache.py +++ b/tests/util/caches/test_responsecache.py @@ -1,4 +1,4 @@ -# Copyright 2021 Vector Creations Ltd +# Copyright 2021 The Matrix.org Foundation C.I.C. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -12,33 +12,39 @@ # See the License for the specific language governing permissions and # limitations under the License. -from twisted.internet.task import Clock - -from synapse.util import Clock as SynapseClock from synapse.util.caches.response_cache import ResponseCache +from tests.server import get_clock from tests.unittest import TestCase -# few notes about test naming here: -# 'wait': denotes tests that have an element of "waiting" before its wrapped result becomes available -# 'expire': denotes tests that test expiry after assured existence - class DeferredCacheTestCase(TestCase): + """ + A TestCase class for ResponseCache. + + The test-case function naming has some logic to it in it's parts, here's some notes about it: + wait: Denotes tests that have an element of "waiting" before its wrapped result becomes available + (Generally these just use .delayed_return instead of .instant_return in it's wrapped call.) + expire: Denotes tests that test expiry after assured existence. + (These have cache with a short timeout_ms=, shorter than will be tested through advancing the clock) + """ + def setUp(self): - self.reactor = Clock() - self.synapse_clock = SynapseClock(self.reactor) + self.reactor, self.clock = get_clock() + + def with_cache(self, name: str, ms: int = 0) -> ResponseCache: + return ResponseCache(self.clock, name, timeout_ms=ms) @staticmethod async def instant_return(o: str) -> str: return o async def delayed_return(self, o: str) -> str: - await self.synapse_clock.sleep(1) + await self.clock.sleep(1) return o def test_cache_hit(self): - cache = ResponseCache(self.synapse_clock, "keeping_cache", timeout_ms=9001) + cache = self.with_cache("keeping_cache", ms=9001) expected_result = "howdy" @@ -56,7 +62,7 @@ def test_cache_hit(self): ) def test_cache_miss(self): - cache = ResponseCache(self.synapse_clock, "trashing_cache", timeout_ms=0) + cache = self.with_cache("trashing_cache", ms=0) expected_result = "howdy" @@ -70,7 +76,7 @@ def test_cache_miss(self): self.assertIsNone(cache.get(0), "cache should not have the result now") def test_cache_expire(self): - cache = ResponseCache(self.synapse_clock, "short_cache", timeout_ms=1000) + cache = self.with_cache("short_cache", ms=1000) expected_result = "howdy" @@ -89,7 +95,7 @@ def test_cache_expire(self): self.assertIsNone(cache.get(0), "cache should not have the result now") def test_cache_wait_hit(self): - cache = ResponseCache(self.synapse_clock, "neutral_cache") + cache = self.with_cache("neutral_cache") expected_result = "howdy" @@ -102,7 +108,7 @@ def test_cache_wait_hit(self): self.assertEqual(expected_result, self.successResultOf(wrap_d)) def test_cache_wait_expire(self): - cache = ResponseCache(self.synapse_clock, "short_cache", timeout_ms=3000) + cache = self.with_cache("medium_cache", ms=3000) expected_result = "howdy" From ecac6adc2ace6aabc0c0dab3ed6edb9baa46305a Mon Sep 17 00:00:00 2001 From: Jonathan de Jong Date: Mon, 8 Mar 2021 18:36:23 +0100 Subject: [PATCH 8/8] whoops, wrong less-than/greater-than indicator on comment --- tests/util/caches/test_responsecache.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/util/caches/test_responsecache.py b/tests/util/caches/test_responsecache.py index a429a62b53ae..f9a187b8defc 100644 --- a/tests/util/caches/test_responsecache.py +++ b/tests/util/caches/test_responsecache.py @@ -125,7 +125,7 @@ def test_cache_wait_expire(self): "cache should still have the result", ) - # (1 + 1 + 2) < 3.0, cache eviction timer is handled + # (1 + 1 + 2) > 3.0, cache eviction timer is handled self.reactor.pump((2,)) self.assertIsNone(cache.get(0), "cache should not have the result now")