-
Notifications
You must be signed in to change notification settings - Fork 411
MSC4360 Sliding Sync threads extension #4360
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
// A `BundledThreadEvent` (as outlined in https://spec.matrix.org/v1.15/client-server-api/#server-side-aggregation-of-mthread-relationships) | ||
// is a thread root event which contains the `m.thread` aggregation included under the | ||
// `m.relations` property in the `unsigned` field of the event. | ||
"thread_root": BundledThreadEvent, |
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 this be optional similar to the new sliding sync extension response? Or even removed from this response altogether?
The token is either acquired from a previous `/thread_updates` response, | ||
or the `prev_batch` in a Sliding Sync response. \ |
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.
Clarify part of the thread extension in the Sliding Sync response
// Optional. Only present in the response if the client missed some events, i.e. there | ||
// was at least one other event in the thread, in addition to the latest event. | ||
// In other words, the `prev_batch` points to the prior-to-latest event. | ||
"prev_batch": "OPAQUE_TOKEN", |
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.
Feels like this should be present no matter what. A server could come online and have some messages in between which we then later want to paginate.
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.
Hmm I'm not sure I see the use case you are talking about. Can you spell out the specifics of when this might occur?
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.
I think was thinking of the backfill scenario. But I guess backfilled messages wouldn't be considered updates since they are historical ⏩
And for any new messages since a server joined, the server should be doing /send
transactions so those messages will show up as thread updates.
The following is a general existing problem as far as I can tell but perhaps would be good more my understanding if I had an explanation here (segregated networks). I'm a bit confused on what would happen if you had servers A, B, C where B and C can't talk to each other directly. A new message from B will flow to A normally via /send
transactions. And then the only way I could see the new message flowing from B to C is if C backfilled from A. But C has no reason to backfill so it probably won't see any of those messages 🤔. Is that expected?
And even if it did backfill, they are backfilled messages so they would never been seen as thread updates. And I don't see how the client would know to re-paginate anything to show the full view of the thread. There weren't even any gaps when the client originally paginated the thread so indicating gaps in the timeline wouldn't help either.
flowchart TD
A <--> B
A <--> C
Perhaps an even simpler scenario is if a bunch of activity occurred on one server and then it tries to /send
it to the other server but it's is offline (or receives a 429 rate-limited response by the other server) and backs-off and gives up. In this case, we have a mechanism in Synapse to try to /send
again later (wake-up destinations that need catchup).
threads have updates without having to paginate through the entire timeline of events. | ||
|
||
|
||
### Sliding Sync extension |
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.
I'm failing to see the value in the Sliding Sync extension.
It seems like someone could just paginate GET /_matrix/client/v1/thread_updates
directly to get the same value.
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.
They would need to continually query /thread_updates
in that case to get the same behaviour of not missing new thread updates, effectively running an additional but separate sync-style loop on /thread_updates
that is specific to obtaining thread updates.
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.
Here is how I'm thinking about it right now: /sync
provides all messages as they come in. If there is a gap (limited: true
), then people can use /thread_updates
.
Am I missing something?
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.
in the scenario of limited: true
from the sliding sync response, this MSC would allow for any additional thread events that weren't included in the regular sync response to be included in the threads extension portion.
So you save yourself needing to go and do the extra query to /thread_updates
(unless you are also limited on thread updates as well in the extension response)
ie.
Sync response covers events A-Z
Sync response only returns events A-M
The threads extension will include any threads that have updates caused by events N-Z.
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.
So yes - you could just go and hit the new endpoint.
But this allows you not to need to do that, and to just receive those updates directly in the sync response you already have.
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.
This reminds me of my suggestion for sticky events. In both of these cases, we are trying to tackle the same problem of gathering a subset of the events in the timeline
:
I think the better way to go about is to apply the same pattern that we just worked through with thread subscriptions. We would need a few things:
- A way to return the new sticky events in
/sync
- Dedicated
sticky_events_limited
flag when there are other sticky events that we didn't return- A pagination endpoint for the sticky events
To be more detailed, these could be normal
timeline
events. If there are more sticky events in between the given sync token and the current position, we set thesticky_events_limited
flag. For the pagination endpoint, we could overload/messages
with a new filter.
With some more thinking on the subject, the dedicated Sliding Sync extension worked well for thread_subscriptions
because thread_subscriptions
aren't part of the response already.
Whereas with sticky events and thread updates, they are already part of the timeline
.
To break down the list of things needed (as listed above) and my current thinking:
"A way to return the new sticky events in /sync
"
In the case of threads and sticky events, these events are already included as part of the timeline
and /sync
already keeps you up to date with all of the events.
"A pagination endpoint for the sticky events"
The dedicated /threads_update
makes sense to back-paginate the gaps (whenever the timeline
is limited: true
)
"Dedicated sticky_events_limited
flag when there are other sticky events that we didn't return"
With more thinking, I think this one is optional.
We could have an extension that indicates whether thread_updates_limited
but this only saves a few extra requests in the cases where the timeline
is limited: true
but there weren't actually any new thread updates in the gap.
Having another field in the Sliding Sync extension to include more thread updates in initial and gappy syncs isn't that useful (and makes things more complicated). You can just call /thread_updates
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.
@bnjbvr Would you be able to comment here on your experience with client-side thread syncing and whether what @MadLittleMods is suggesting would be better or worse?
|
||
### Companion endpoint for backpaginating thread updates across all rooms | ||
|
||
A new `/thread_updates` endpoint is added to allow a client to obtain missing thread updates for a client. |
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.
What is a thread update?
Does it include edits to messages in the thread? Edits to the thread root (and maybe the latest message in thread) are mentioned in the MSC but it's unclear about messages in the middle. Especially in the case where your client has already paginated the thread, a period of time passes leaving a gap with some edits to the thread messages and you /sync
or use this endpoint again.
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.
A thread update is simply any event which has a m.thread
relation. This doesn't currently include edits to any of the thread events. I'm not sure if edits would be particularly useful to clients, whereas new events in a given thread is a clear and meaningful change to a thread that a user would want to be notified about.
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.
How do we expect a client to show the most recent view of a thread (with edits and other bundled aggregations)?
Imagine the scenario where you have kept up to date with all of the messages in a thread, then you go offline, some edits and reactions occur to the thread messages, more unrelated activity occurs to fill up the /sync
response which leaves the edits/reactions in the gap and not included in the timeline
when you come back online and /sync
again.
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.
I'm not sure what the best approach should be. I can see an argument where clients may or may not want to include events and/or annotations as "thread updates".
We could extend the MSC to include edits & annotations as "thread updates", but if we did that we may want to extend the extension config to make them optional, or provide a better way for clients to define what they want to be considered as a "thread update".
@bnjbvr Can you weigh in here on what clients would want?
in the user's joined rooms. The new `extension` provides a mechanism for clients to quickly and easily "catch up" to any | ||
missed thread updates. |
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.
In a similar regard to "what is a thread update", how do we expect people to get thread activity for historical rooms (they were previously joined and are no longer joined)? They should be able to just as easily spider out their interested threads.
This includes the case where they are newly_left
within the batch.
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.
I don't know if clients would care about getting "updates" for rooms they are no longer following.
The intent of the new API is to allow clients to follow along with new updates that are relevant. This is especially evident with the API design of only returning (room_id, thread_id)
pairs in the response to indicate that a particular thread had at least one update in that window. The APIs are not meant to be able to query all thread events.
If a client wants historical thread events, they should use the /relations
endpoint to retrieve them.
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.
Once you're left, using /threads
makes sense to find threads in the room and GET /relations/{threadRootId}/m.thread
to find all events in the thread.
I think this just needs to be clarified that you should still get updates if you have a Sliding Sync pos
token where you are joined to the room, some thread updates occur, you leave, and then you do another incremental sync (newly_left
). You should get thread updates until you left.
threads have updates without having to paginate through the entire timeline of events. | ||
|
||
|
||
### Sliding Sync extension |
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.
Clarify what this includes in the case of initial sync
Depends-on: #4186
Rendered