-
Notifications
You must be signed in to change notification settings - Fork 187
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
SS: Reset connection if token is unrecognized #17529
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 @@ | ||
Reset the sliding sync connection if we don't recognize the per-connection state position. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,6 +47,7 @@ | |
EventTypes, | ||
Membership, | ||
) | ||
from synapse.api.errors import SlidingSyncUnknownPosition | ||
from synapse.events import EventBase, StrippedStateEvent | ||
from synapse.events.utils import parse_stripped_state_event, strip_event | ||
from synapse.handlers.relations import BundledAggregations | ||
|
@@ -491,6 +492,22 @@ async def current_sync_for_user( | |
# See https://github.com/matrix-org/matrix-doc/issues/1144 | ||
raise NotImplementedError() | ||
|
||
if from_token: | ||
# Check that we recognize the connection position, if not tell the | ||
# clients that they need to start again. | ||
Comment on lines
+496
to
+497
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. Add the context for why we do this:
-> We do this to avoid sending down all rooms and their state from scratch (
Comment on lines
+496
to
+497
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. Is the following explanation correct? Before this change, if the With this new change, we're forcing them to send another initial sync request where we will also return rooms with We should consider adding this context as well. comment exampleThis allows the client a chance to do an initial request with a smaller range of rooms to get them some results sooner but will end up taking the same amount of time (more with round-trips and re-processing) in the end to get everything again. 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. Yup! |
||
# | ||
# If we don't do this and the client asks for the full range of | ||
# rooms, we end up sending down all rooms and their state from | ||
# scratch (which can be very slow). By expiring the connection we | ||
# allow the client a chance to do an initial request with a smaller | ||
# range of rooms to get them some results sooner but will end up | ||
# taking the same amount of time (more with round-trips and | ||
# re-processing) in the end to get everything again. | ||
if not await self.connection_store.is_valid_token( | ||
sync_config, from_token.connection_position | ||
): | ||
raise SlidingSyncUnknownPosition() | ||
|
||
await self.connection_store.mark_token_seen( | ||
sync_config=sync_config, | ||
from_token=from_token, | ||
|
@@ -2821,6 +2838,16 @@ class SlidingSyncConnectionStore: | |
attr.Factory(dict) | ||
) | ||
|
||
async def is_valid_token( | ||
self, sync_config: SlidingSyncConfig, connection_token: int | ||
) -> bool: | ||
"""Return whether the connection token is valid/recognized""" | ||
if connection_token == 0: | ||
return True | ||
|
||
conn_key = self._get_connection_key(sync_config) | ||
return connection_token in self._connections.get(conn_key, {}) | ||
|
||
async def have_sent_room( | ||
self, sync_config: SlidingSyncConfig, connection_token: int, room_id: str | ||
) -> HaveSentRoom: | ||
|
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 spec calls this "expiring connections" vs "resetting connections". Probably should just align all of the language to the spec because it also explains how "To handle expired connections".
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 still says "reset")
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.
oh nyaryiyr, will fix. (i thought I had got them all 🙁 )
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.
#17531