-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Properly handle unknown results for the stream change cache. #14592
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Fix a long-standing bug where a device list update might not be sent to clients in certain circumstances. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -842,12 +842,11 @@ async def get_users_whose_devices_changed( | |
user_ids, from_key | ||
) | ||
|
||
if not user_ids_to_check: | ||
# If an empty set was returned, there's nothing to do. | ||
if user_ids_to_check is not None and not user_ids_to_check: | ||
return set() | ||
|
||
def _get_users_whose_devices_changed_txn(txn: LoggingTransaction) -> Set[str]: | ||
changes: Set[str] = set() | ||
|
||
stream_id_where_clause = "stream_id > ?" | ||
sql_args = [from_key] | ||
|
||
|
@@ -858,19 +857,25 @@ def _get_users_whose_devices_changed_txn(txn: LoggingTransaction) -> Set[str]: | |
sql = f""" | ||
SELECT DISTINCT user_id FROM device_lists_stream | ||
WHERE {stream_id_where_clause} | ||
AND | ||
""" | ||
|
||
# Query device changes with a batch of users at a time | ||
# Assertion for mypy's benefit; see also | ||
# https://mypy.readthedocs.io/en/stable/common_issues.html#narrowing-and-inner-functions | ||
assert user_ids_to_check is not None | ||
for chunk in batch_iter(user_ids_to_check, 100): | ||
clause, args = make_in_list_sql_clause( | ||
txn.database_engine, "user_id", chunk | ||
) | ||
txn.execute(sql + clause, sql_args + args) | ||
changes.update(user_id for user_id, in txn) | ||
# If the stream change cache gave us no information, fetch *all* | ||
# users between the stream IDs. | ||
if user_ids_to_check is None: | ||
txn.execute(sql, sql_args) | ||
return {user_id for user_id, in txn} | ||
|
||
# Otherwise, fetch changes for the given users. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to additionally query the database if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I don't think we need to do that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bit more complicated than initially thought as the database query is what is used to cap things at a max stream token (i.e. we only check the cache for if things have changed after |
||
else: | ||
changes: Set[str] = set() | ||
|
||
# Query device changes with a batch of users at a time | ||
for chunk in batch_iter(user_ids_to_check, 100): | ||
clause, args = make_in_list_sql_clause( | ||
txn.database_engine, "user_id", chunk | ||
) | ||
txn.execute(sql + " AND " + clause, sql_args + args) | ||
changes.update(user_id for user_id, in txn) | ||
|
||
return changes | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_filter_app_presence_updates_for_user
is only called ifadditional_users_interested_in == PresenceRouter.ALL_USERS
, so it seems reasonable here to treated this the same as not having afrom_key
(and returning info on all users).