Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Sliding Sync
/sync/e2eeendpoint for To-Device messages #17167Add Sliding Sync
/sync/e2eeendpoint for To-Device messages #17167Changes from 17 commits
f9e6e531e05a055e925f669f9143d4ff933371ec5706d12e5b8b70bac60a4f810ffae66bf48968871dac0892283adb7e20f0983556b7cfd7b9e53799bdfa167331401b23abca821a1b335ca9374ad7a8b3092ab53539abe5f194f902cecfaf6122ff2f112e7c2221bb717b160514aba5974979506ac1da3da6bc1d4b41aa6606ac1ab0b844a482545File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Thinking more on that "relieving pressure on the Sliding Sync proxy now" point. Since the proxy is just another Matrix client which uses the
/syncv2 loop, I don't think this new/sync/e2eeendpoint will help anything because:(
/syncv2 will acknowledge all of the To-Device events we're trying to serve with this new endpoint)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.
🤔 I think I wasn't taking into account that the proxy will be its own device that can be separate from device we can use for this endpoint. So acknowledging the To-Device messages for one device doesn't affect the other.
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.
Discussed this point in a meeting with @erikjohnston today and I think this might be a problem since it will be one device consuming everything. Would probably require updates to the Sliding Sync proxy to utilize both properly.
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 would have thought you'd either have a device using the proxy entirely or you have a device using Synapse's SS impl entirely, not a weird mixture of both. If so, this is not a problem afaict?
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.
The idea of this endpoint isn't to remove pressure from the proxy directly.
By having a new endpoint, the Matrix Rust SDK can consume it directly instead of using sliding sync proxy with a particular
conn_idfor Sliding Sync.That's the same idea with the improved
/context, it removes the need for another use of sliding sync with anotherconn_id.Finally, the last usage of sliding sync is the room list API. Since there will be only one usage of sliding sync, the
conn_idwon't be necessary anymore. In the simplified sliding sync implementation inside synapse, it means you would not have to manage multiple connection contexts at the same time.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 isn't true though, because the proxy will consume to-device messages that would have been delivered via
/sync/e2ee, hence why it has to be an all-or-nothing for E2EE data. For/contextthen absolutely, that can be done via synapse only.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 indeed. So the migration should ideally be all-on-synapse, instead of half-proxy, half-synapse. Gotcha.
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.
You could probably use a whole another device for all e2ee operations, but it's quite ugly and a workaround.
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 lot of this is just sharing and summarizing the clarifications from @erikjohnston 🙇
Discussed this point a bit further and got a little more clarification. The main aim of
/sync/e2eeand/context[2] changes is to make handling notifications easier for the clients. Being able to use/sync/e2eealongside the Sliding Sync proxy to relieve pressure was just an extra thought but given that the proxy and client share the same device [1], will not work (see To-Device problem explained above). If usage of/syncv2 and/sync/e2eeusage is split between separate devices (as @MatMaul mentioned) it would work but I don't think people have bandwidth/desire/benefit to do it.This means that this endpoint probably won't be useful until we have a Sliding Sync implementation in Synapse itself. I started working on
/sync/e2eebecause I thought it would have some "sliding" aspect to ease my way into the concept but turns out to be just some/syncv2 refactoring. It's still good in terms of getting familiar with/syncv2 though.It may very well turn out that
/sync/e2eeis not actually as helpful as we thought (or ifdevice_listsis sufficiently heavyweight that its just as slow), but from @erikjohnston's understanding right now, it'd be quite helpful to the SDK team (I don't know the exact details here).[1] Sliding sync proxy shares the same device as the ElementX client (as far as it has been explained to me). The way it works is that EX calls the SS proxy with that token, and then the SS proxy caches it to make the requests.
[2] For onlookers, the
/contextchanges just entail making the endpoint work for all events like invites. I haven't looked into what's necessary but it may be as simple as adjusting the check for whether someone is joined to the room.