Skip to content
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

MSC3575: Sliding Sync (aka Sync v3) #3575

Open
wants to merge 81 commits into
base: main
Choose a base branch
from
Open

MSC3575: Sliding Sync (aka Sync v3) #3575

wants to merge 81 commits into from

Conversation

kegsay
Copy link
Member

@kegsay kegsay commented Dec 20, 2021

@kegsay kegsay marked this pull request as draft December 20, 2021 10:49
@ara4n ara4n added the proposal A matrix spec change proposal label Dec 20, 2021
@turt2live turt2live added needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. kind:core MSC which is critical to the protocol's success client-server Client-Server API labels Dec 20, 2021
proposals/3575-sync.md Outdated Show resolved Hide resolved
proposals/3575-sync.md Outdated Show resolved Hide resolved
proposals/3575-sync.md Outdated Show resolved Hide resolved
proposals/3575-sync.md Outdated Show resolved Hide resolved
proposals/3575-sync.md Outdated Show resolved Hide resolved
proposals/3575-sync.md Outdated Show resolved Hide resolved
proposals/3575-sync.md Outdated Show resolved Hide resolved
proposals/3575-sync.md Outdated Show resolved Hide resolved
proposals/3575-sync.md Outdated Show resolved Hide resolved
proposals/3575-sync.md Outdated Show resolved Hide resolved
proposals/3575-sync.md Outdated Show resolved Hide resolved
proposals/3575-sync.md Outdated Show resolved Hide resolved
@turt2live turt2live removed their request for review June 6, 2023 19:50
proposals/3575-sync.md Outdated Show resolved Hide resolved
```json5
{
"unsubscribe_rooms": [ "!sub1:bar" ]
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit concerned that this API will make it easy for clients to "forget" to unsubscribe from rooms, and you'd end up with clients slowly racking up lots of room subscriptions. Personally, I'd be tempted to change this such that every request you need to specify which rooms the client wants to remain subscribed to 🤷

@Hywan
Copy link
Member

Hywan commented Aug 16, 2023

timestamp has been added onto SlidingSyncResponse's room.$room_id, matrix-org/sliding-sync#247.

@DMRobertson
Copy link
Contributor

Is the returned required_state that at the end or the start of the timeline? (GH is refusing to render the pull request changes, so I can't leave a nice review comment, sorry.)

@kegsay
Copy link
Member Author

kegsay commented Nov 17, 2023

The end. It is the current state.

@turt2live turt2live changed the title MSC3575 Sliding Sync (aka Sync v3) MSC3575: Sliding Sync (aka Sync v3) Mar 20, 2024
There are three main reasons why a client may want to have >1 connection to the server open concurrently:
- The client is a **browser**, and it should be possible to open the same client in multiple tabs without causing problems. Without concurrent connections, each tab would reset the other tabs connection due to different `?pos=` values being sent. The number of concurrent connections is technically unbounded.
- The client is a **mobile application**, and it should be possible to have a "push process" connection in addition to the "app connection". Without concurrent connections, it isn't possible to obtain to-device messages in the push process, whilst also obtaining them in the main app. The number of concurrent connections is fixed e.g 2.
- The client wants to do a **one-shot request** for some data, without incurring latency/bandwidth penalties with all the activity on the user's account. Without concurrent connections, it isn't possible to get the response without also potentially getting large amounts of extraneous data. The number of concurrent connections is N+1, where N is the number of active concurrent connections.
Copy link
Contributor

Choose a reason for hiding this comment

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

The number of concurrent connections is N+1, where N is the number of active concurrent connections.

What is this trying to convey (can someone re-word)? I'm reading this as "a dog is a dog"

Comment on lines +432 to +434
// If the child room has a m.room.tombstone event, then the search should recursively navigate
// the room ID in that event to find the latest room and use that room ID instead of the initial
// room ID in the m.space.child event.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we want to recursively find the latest room? What benefit for this extra work?

It seems like the new room would also be part of the space. If the user has already joined the replacement room, then the old one doesn't even show up (depending on include_old_rooms). If the user hasn't joined the replacement room, they can accept their invite or visit the old room and see the pointer to the new place to join. (see Tombstones section)

Copy link
Member

Choose a reason for hiding this comment

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

It seems like the new room would also be part of the space.

Only if the user upgrading had permissions to manage the space (and was on a client e.g. EW/ED which had this logic) - quite a lot of spaces are soft-broken where they only know about old versions of rooms.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like if the spaces still know about the old rooms, people will visit the old room, be pointed to the new one and can ask the admin to fix space/room relationship.

Copy link
Member

Choose a reason for hiding this comment

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

@MadLittleMods sure, except the space admin might not even be in the room for them to as ask. So they'll likely end up asking the wrong admin.

Comment on lines +1517 to +1519
Whilst this in MSC review the HTTP path will be `/_matrix/client/unstable/org.matrix.msc3575/sync`
with the intention of this eventually becoming (confusingly) `/_matrix/client/v4/sync`. As this is a
brand new endpoint, no other keys or fields need prefixing.
Copy link
Member

Choose a reason for hiding this comment

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

Currently there is no good way for a client to detect presence of a server's support for this MSC unless they are supporting it via proxy (detected via well-known).

If a client tries to make a request via POST to the above sync endpoint and the environment is actually running a proxy on the same subdomain then you end up losing to_device messages as the proxy starts a new legacy sync stream against the same access token.

Can we please add an entry to https://spec.matrix.org/v1.10/client-server-api/#get_matrixclientversions unstable_features to enable the server to express that it has native support.

Workarounds I tried:

OPTIONS request: 200 from the proxy, 204 from Synapse itself
HEAD request: 405 from the proxy, 404 from Synapse itself

Copy link
Member

Choose a reason for hiding this comment

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

In lieu of the MSC suggesting something itself, I am assuming org.matrix.msc3575 will be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:core MSC which is critical to the protocol's success matrix-2.0 Required for Matrix 2.0 proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet