Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Do not assume calls to runInteraction return Deferreds. #8133

Merged
merged 4 commits into from
Aug 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/8133.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Convert various parts of the codebase to async/await.
7 changes: 3 additions & 4 deletions synapse/crypto/keyring.py
Original file line number Diff line number Diff line change
Expand Up @@ -757,9 +757,8 @@ async def get_key(key_to_fetch_item):
except Exception:
logger.exception("Error getting keys %s from %s", key_ids, server_name)

return await yieldable_gather_results(
get_key, keys_to_fetch.items()
).addCallback(lambda _: results)
await yieldable_gather_results(get_key, keys_to_fetch.items())
return results

async def get_server_verify_key_v2_direct(self, server_name, key_ids):
"""
Expand All @@ -769,7 +768,7 @@ async def get_server_verify_key_v2_direct(self, server_name, key_ids):
key_ids (iterable[str]):

Returns:
Deferred[dict[str, FetchKeyResult]]: map from key ID to lookup result
dict[str, FetchKeyResult]: map from key ID to lookup result

Raises:
KeyLookupError if there was a problem making the lookup
Expand Down
10 changes: 7 additions & 3 deletions synapse/module_api/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,10 @@ def record_user_external_id(
external_id: id on that system
user_id: complete mxid that it is mapped to
"""
return self._store.record_user_external_id(
auth_provider_id, remote_user_id, registered_user_id
return defer.ensureDeferred(
self._store.record_user_external_id(
auth_provider_id, remote_user_id, registered_user_id
)
Comment on lines +170 to +173
Copy link
Member Author

@clokep clokep Aug 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have been in #8100.

)

def generate_short_term_login_token(
Expand Down Expand Up @@ -223,7 +225,9 @@ def run_db_interaction(self, desc, func, *args, **kwargs):
Returns:
Deferred[object]: result of func
"""
return self._store.db_pool.runInteraction(desc, func, *args, **kwargs)
return defer.ensureDeferred(
self._store.db_pool.runInteraction(desc, func, *args, **kwargs)
)

def complete_sso_login(
self, registered_user_id: str, request: SynapseRequest, client_redirect_url: str
Expand Down
6 changes: 4 additions & 2 deletions synapse/spam_checker_api/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,10 @@ def get_state_events_in_room(self, room_id: str, types: tuple) -> defer.Deferred
twisted.internet.defer.Deferred[list(synapse.events.FrozenEvent)]:
The filtered state events in the room.
"""
state_ids = yield self._store.get_filtered_current_state_ids(
room_id=room_id, state_filter=StateFilter.from_types(types)
state_ids = yield defer.ensureDeferred(
self._store.get_filtered_current_state_ids(
room_id=room_id, state_filter=StateFilter.from_types(types)
)
)
state = yield defer.ensureDeferred(self._store.get_events(state_ids.values()))
return state.values()
7 changes: 4 additions & 3 deletions synapse/storage/databases/main/group_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -341,14 +341,15 @@ def _get_users_for_summary_txn(txn):
"get_users_for_summary_by_role", _get_users_for_summary_txn
)

def is_user_in_group(self, user_id, group_id):
return self.db_pool.simple_select_one_onecol(
async def is_user_in_group(self, user_id: str, group_id: str) -> bool:
result = await self.db_pool.simple_select_one_onecol(
table="group_users",
keyvalues={"group_id": group_id, "user_id": user_id},
retcol="user_id",
allow_none=True,
desc="is_user_in_group",
).addCallback(lambda r: bool(r))
)
return bool(result)

def is_user_admin_in_group(self, group_id, user_id):
return self.db_pool.simple_select_one_onecol(
Expand Down
28 changes: 16 additions & 12 deletions synapse/storage/databases/main/keys.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import itertools
import logging
from typing import Iterable, Tuple

from signedjson.key import decode_verify_key_bytes

Expand Down Expand Up @@ -88,12 +89,17 @@ def _txn(txn):

return self.db_pool.runInteraction("get_server_verify_keys", _txn)

def store_server_verify_keys(self, from_server, ts_added_ms, verify_keys):
async def store_server_verify_keys(
self,
from_server: str,
ts_added_ms: int,
verify_keys: Iterable[Tuple[str, str, FetchKeyResult]],
) -> None:
"""Stores NACL verification keys for remote servers.
Args:
from_server (str): Where the verification keys were looked up
ts_added_ms (int): The time to record that the key was added
verify_keys (iterable[tuple[str, str, FetchKeyResult]]):
from_server: Where the verification keys were looked up
ts_added_ms: The time to record that the key was added
verify_keys:
keys to be stored. Each entry is a triplet of
(server_name, key_id, key).
"""
Expand All @@ -115,13 +121,7 @@ def store_server_verify_keys(self, from_server, ts_added_ms, verify_keys):
# param, which is itself the 2-tuple (server_name, key_id).
invalidations.append((server_name, key_id))

def _invalidate(res):
f = self._get_server_verify_key.invalidate
for i in invalidations:
f((i,))
return res
Comment on lines -118 to -122
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simple_upsert_many_txn returns None, so the input to the callback (res) was always None.

There's no reason to store this and return it, it makes it more confusing.


return self.db_pool.runInteraction(
await self.db_pool.runInteraction(
"store_server_verify_keys",
self.db_pool.simple_upsert_many_txn,
table="server_signature_keys",
Expand All @@ -134,7 +134,11 @@ def _invalidate(res):
"verify_key",
),
value_values=value_values,
).addCallback(_invalidate)
)

invalidate = self._get_server_verify_key.invalidate
for i in invalidations:
invalidate((i,))

def store_server_keys_json(
self, server_name, key_id, from_server, ts_now_ms, ts_expires_ms, key_json_bytes
Expand Down
13 changes: 6 additions & 7 deletions synapse/storage/databases/main/user_erasure_store.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,30 +13,29 @@
# See the License for the specific language governing permissions and
# limitations under the License.

import operator

from synapse.storage._base import SQLBaseStore
from synapse.util.caches.descriptors import cached, cachedList


class UserErasureWorkerStore(SQLBaseStore):
@cached()
def is_user_erased(self, user_id):
async def is_user_erased(self, user_id: str) -> bool:
"""
Check if the given user id has requested erasure

Args:
user_id (str): full user id to check
user_id: full user id to check

Returns:
Deferred[bool]: True if the user has requested erasure
True if the user has requested erasure
"""
return self.db_pool.simple_select_onecol(
result = await self.db_pool.simple_select_onecol(
table="erased_users",
keyvalues={"user_id": user_id},
retcol="1",
desc="is_user_erased",
).addCallback(operator.truth)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

operator.truth is equivalent to bool, I don't know why bool wasn't just used here in the first place.

)
return bool(result)

@cachedList(cached_method_name="is_user_erased", list_name="user_ids")
async def are_users_erased(self, user_ids):
Expand Down