-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Send to-device messages to application services #11215
Changes from 16 commits
7fbfedb
b7a44d4
78bd5ea
7899f82
103f410
e914f1d
2930fe6
f65846b
ce020c3
8f1183c
401cb2b
179dd5a
8b0bbc1
31c4b40
bd9d963
8f8226a
b4a4b45
c691ef0
7cf6ad9
6d68b8a
13b25cf
275e1e0
403490d
385b3bf
c0b157d
ba91438
0685021
e7f6732
3d1661f
0ac079b
822e92a
25488fa
026cb8a
1121674
8129657
de48ab4
d8b8f74
ced1314
c749fcb
24512fb
24bc3c5
30b74a5
80c3721
3d8e50d
50ebcb9
089e041
d06781e
c334eef
bf93ec4
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 @@ | ||
Add experimental support for sending to-device messages to application services, as specified by [MSC2409](https://github.com/matrix-org/matrix-doc/pull/2409). Disabled by default. | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,7 @@ | |
# limitations under the License. | ||
|
||
import logging | ||
from typing import TYPE_CHECKING, List, Optional, Tuple | ||
from typing import TYPE_CHECKING, Collection, Dict, List, Optional, Tuple | ||
|
||
from synapse.logging import issue9533_logger | ||
from synapse.logging.opentracing import log_kv, set_tag, trace | ||
|
@@ -24,6 +24,7 @@ | |
DatabasePool, | ||
LoggingDatabaseConnection, | ||
LoggingTransaction, | ||
make_in_list_sql_clause, | ||
) | ||
from synapse.storage.engines import PostgresEngine | ||
from synapse.storage.util.id_generators import ( | ||
|
@@ -136,6 +137,79 @@ def process_replication_rows(self, stream_name, instance_name, token, rows): | |
def get_to_device_stream_token(self): | ||
return self._device_inbox_id_gen.get_current_token() | ||
|
||
async def get_new_messages( | ||
anoadragon453 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
self, | ||
user_ids: Collection[str], | ||
from_stream_id: int, | ||
to_stream_id: int, | ||
) -> Dict[Tuple[str, str], List[JsonDict]]: | ||
""" | ||
Retrieve to-device messages for a given set of user IDs. | ||
|
||
Only to-device messages with stream ids between the given boundaries | ||
(from < X <= to) are returned. | ||
|
||
Note that a stream ID can be shared by multiple copies of the same message with | ||
different recipient devices. Each (device, message_content) tuple has their own | ||
row in the device_inbox table. | ||
|
||
Args: | ||
user_ids: The users to retrieve to-device messages for. | ||
from_stream_id: The lower boundary of stream id to filter with (exclusive). | ||
to_stream_id: The upper boundary of stream id to filter with (inclusive). | ||
|
||
Returns: | ||
A list of to-device messages. | ||
""" | ||
# Bail out if none of these users have any messages | ||
for user_id in user_ids: | ||
if self._device_inbox_stream_cache.has_entity_changed( | ||
user_id, from_stream_id | ||
): | ||
break | ||
else: | ||
return {} | ||
|
||
def get_new_messages_txn(txn: LoggingTransaction): | ||
# Build a query to select messages from any of the given users that are between | ||
# the given stream id bounds | ||
|
||
# Scope to only the given users. We need to use this method as doing so is | ||
# different across database engines. | ||
many_clause_sql, many_clause_args = make_in_list_sql_clause( | ||
self.database_engine, "user_id", user_ids | ||
anoadragon453 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
|
||
sql = f""" | ||
SELECT user_id, device_id, message_json FROM device_inbox | ||
WHERE {many_clause_sql} | ||
AND ? < stream_id AND stream_id <= ? | ||
ORDER BY stream_id ASC | ||
anoadragon453 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
""" | ||
|
||
txn.execute(sql, (*many_clause_args, from_stream_id, to_stream_id)) | ||
|
||
# Create a dictionary of (user ID, device ID) -> list of messages that | ||
# that device is meant to receive. | ||
recipient_user_id_device_id_to_messages: Dict[ | ||
Tuple[str, str], List[JsonDict] | ||
] = {} | ||
|
||
for row in txn: | ||
recipient_user_id = row[0] | ||
recipient_device_id = row[1] | ||
message_dict = db_to_json(row[2]) | ||
|
||
recipient_user_id_device_id_to_messages.setdefault( | ||
(recipient_user_id, recipient_device_id), [] | ||
).append(message_dict) | ||
|
||
return recipient_user_id_device_id_to_messages | ||
|
||
return await self.db_pool.runInteraction( | ||
"get_new_messages", get_new_messages_txn | ||
) | ||
|
||
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. I'm rather uncomfortable that we're duplicating 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. We could, though querying by devices rather than user IDs can result in a larger query. Though, Travis tells me that it's likely that AS users will typically only have 1 device per user, maybe 2. But more would mean sending more data to the db server. 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. I'm also happy to just add 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. Given the above thread on how querying with device IDs is more efficient, combining these two methods makes sense. However, there's an annoying subtlety here which makes it difficult to support a Stream IDs for to-device messages are only unique to
Note mainly that there are multiple rows with the same stream_id - but there's only ever one The This is why I dropped All that being said though, we could just have public methods which only accept:
and both call to a unified, private method. How does that sound? 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.
I'm not sure I entirely follow this. As long as you 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. What will the caller than call the method with next time though? If they try again with the last returned An alternative approach: The caller provides This way, if we end up hitting our limit in the middle of a We'd need to ensure a I think that should work, though there's likely a simpler way to do it... 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. Following on from your example; suppose we call
We can now tell that, at the limit (2), we would have incomplete results for Next time round, the caller passes the returned (You might remember this algorithm from https://github.com/matrix-org/synapse/pull/5156/files#diff-d9219a3564a55443579532e051d6abc32f47e8f4ba44c64459cd4f9d378e22bdR90-R121). You're right that it fails if there are more messages with a given 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.
Pretty likely I'm afraid - current end-to-end encryption bridge architectures require at least one device per bridged user. 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. bleh. In which case, maybe we're better off with your earlier idea of
having to store an offset into a given stream_id feels pretty icky to me. 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. 822e92a refactors the two previous methods into two public methods ( A |
||
async def get_new_messages_for_device( | ||
self, | ||
user_id: str, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
/* Copyright 2021 The Matrix.org Foundation C.I.C | ||
anoadragon453 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* | ||
* 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. | ||
*/ | ||
|
||
-- Add a column to track what to_device stream id that this application | ||
-- service has been caught up to. | ||
ALTER TABLE application_services_state ADD COLUMN to_device_stream_id BIGINT; | ||
richvdh marked this conversation as resolved.
Show resolved
Hide resolved
|
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.
Should we note the experimental configuration flag to enable this?