From d5fe5f9f4d1e5f0aa93a3d2488b4c2d976650204 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 24 May 2019 15:20:24 +0100 Subject: [PATCH 1/2] Simplification to Keyring.wait_for_previous_lookups. The list of server names was redundant, since it was equivalent to the keys on the server_to_deferred map. This reduces the number of large lists being passed around, and has the benefit of deduplicating the entries in `wait_on`. --- changelog.d/5250.misc | 1 + synapse/crypto/keyring.py | 11 ++++------- 2 files changed, 5 insertions(+), 7 deletions(-) create mode 100644 changelog.d/5250.misc diff --git a/changelog.d/5250.misc b/changelog.d/5250.misc new file mode 100644 index 000000000000..575a299a8214 --- /dev/null +++ b/changelog.d/5250.misc @@ -0,0 +1 @@ +Simplification to Keyring.wait_for_previous_lookups. diff --git a/synapse/crypto/keyring.py b/synapse/crypto/keyring.py index eaf41b983c11..d6ad7f177260 100644 --- a/synapse/crypto/keyring.py +++ b/synapse/crypto/keyring.py @@ -180,9 +180,7 @@ def _start_key_lookups(self, verify_requests): # We want to wait for any previous lookups to complete before # proceeding. - yield self.wait_for_previous_lookups( - [rq.server_name for rq in verify_requests], server_to_deferred - ) + yield self.wait_for_previous_lookups(server_to_deferred) # Actually start fetching keys. self._get_server_verify_keys(verify_requests) @@ -215,12 +213,11 @@ def remove_deferreds(res, verify_request): logger.exception("Error starting key lookups") @defer.inlineCallbacks - def wait_for_previous_lookups(self, server_names, server_to_deferred): + def wait_for_previous_lookups(self, server_to_deferred): """Waits for any previous key lookups for the given servers to finish. Args: - server_names (list): list of server_names we want to lookup - server_to_deferred (dict): server_name to deferred which gets + server_to_deferred (dict[str, Deferred]): server_name to deferred which gets resolved once we've finished looking up keys for that server. The Deferreds should be regular twisted ones which call their callbacks with no logcontext. @@ -233,7 +230,7 @@ def wait_for_previous_lookups(self, server_names, server_to_deferred): while True: wait_on = [ (server_name, self.key_downloads[server_name]) - for server_name in server_names + for server_name in server_to_deferred.keys() if server_name in self.key_downloads ] if not wait_on: From cbf02e24751caeaccb1d7e4ddceaf5aa39ce1e9a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 24 May 2019 15:40:28 +0100 Subject: [PATCH 2/2] fix tests --- tests/crypto/test_keyring.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/crypto/test_keyring.py b/tests/crypto/test_keyring.py index de61bad15d27..4fba462d4444 100644 --- a/tests/crypto/test_keyring.py +++ b/tests/crypto/test_keyring.py @@ -85,7 +85,7 @@ def test_wait_for_previous_lookups(self): # we run the lookup in a logcontext so that the patched inlineCallbacks can check # it is doing the right thing with logcontexts. wait_1_deferred = run_in_context( - kr.wait_for_previous_lookups, ["server1"], {"server1": lookup_1_deferred} + kr.wait_for_previous_lookups, {"server1": lookup_1_deferred} ) # there were no previous lookups, so the deferred should be ready @@ -94,7 +94,7 @@ def test_wait_for_previous_lookups(self): # set off another wait. It should block because the first lookup # hasn't yet completed. wait_2_deferred = run_in_context( - kr.wait_for_previous_lookups, ["server1"], {"server1": lookup_2_deferred} + kr.wait_for_previous_lookups, {"server1": lookup_2_deferred} ) self.assertFalse(wait_2_deferred.called)