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 3 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
22 changes: 13 additions & 9 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,7 +89,12 @@ 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
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