Non lazy loading sync not blocking during fast join #14831

MatMaul · 2023-01-12T17:38:22Z

Fix #12989.

DMR built on this in #14870

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file.
Pull request includes a sign off
Code style is correct
(run the linters)

Signed-off-by: Mathieu Velten <mathieuv@matrix.org>

DMRobertson

Seems reasonable. Do you have any complement tests for this? (i.e. how do we know this works?)

changelog.d/14831.misc

synapse/types/__init__.py

synapse/storage/databases/main/room.py

synapse/handlers/sync.py

synapse/types/__init__.py

synapse/handlers/sync.py

MatMaul · 2023-01-17T13:11:37Z

Seems reasonable. Do you have any complement tests for this? (i.e. how do we know this works?)

Yes, I've just opened a PR with Complement side of the changes.
This includes a test that was checking sync was blocking, and is now checking that it is NOT blocking and that we still get the joined room after the resync (NonLazySyncDuringPartialStateJoin).

Co-authored-by: David Robertson <davidr@element.io>

DMRobertson

LGTM. Would be good to double-check #14831 (comment) if you get a chance though

squahtx · 2023-01-17T19:15:53Z

Perhaps I have missed something, but when a room is un-partial stated while an incremental sync is waiting for updates, do we wake up the sync?
Also can we have a complement test for it?

squahtx · 2023-01-17T19:18:52Z

also do we want / need to do anything for rooms that become un-partial stated after we leave them?

synapse/handlers/sync.py

MatMaul · 2023-01-18T11:49:11Z

Perhaps I have missed something, but when a room is un-partial stated while an incremental sync is waiting for updates, do we wake up the sync? Also can we have a complement test for it?

Oh sorry I missed your comment. I think I came to the conclusion that it was handled generically by the stream code but I'll double check, and add a test for it indeed, thanks.

DMRobertson · 2023-01-18T16:36:59Z

synapse/handlers/sync.py

@@ -1819,8 +1819,12 @@ async def _generate_sync_entry_for_rooms(

        # Retrieve rooms that got un partial stated in the meantime, only useful in case
        # of a non lazy-loading-members sync.
+        # We also skip calculating that in case of initial sync since we don't need it.


I'm not following you here. I thought we want to exclude partially-joined rooms from all non-lazy load syncs. Why should that not also apply to an initial, non-lazy sync?

We skip calculating the rooms that got unpartialstated because in case of initial, we have another piece of code that already exclude any partial stated rooms. The stream of unpartialstated rooms is only useful to know which ones have been unpartialed between 2 syncs, for an initial sync we just directly check the DB.

MatMaul · 2023-01-18T17:21:16Z

Oh sorry I missed your comment. I think I came to the conclusion that it was handled generically by the stream code but I'll double check, and add a test for it indeed, thanks.

I have a test, and I was wrong, it doesn't work :/ I am trying to fix it right now.

also do we want / need to do anything for rooms that become un-partial stated after we leave them?

I'll put some thoughts into that after fixing the notifying behavior.

DMRobertson

I'm not seeing where we exclude things for partially joined rooms during incremental, non-lazy syncs. C.f. matrix-org/complement#584 (review)

I'm finding this tricky to follow in general. It might be worth pairing tomorrow to try and chop this up into some smaller pieces, if you're up for that

DMRobertson · 2023-01-18T17:38:36Z

synapse/handlers/sync.py

+        # Retrieve rooms that got un partial stated in the meantime, only useful in case
+        # of a non lazy-loading-members sync.
+        # We also skip calculating that in case of initial sync since we don't need it.


Comment: For nonlazy, incremental syncs: find all the rooms we excluded in previous nonlazy syncs that we can now disclose to the user.

DMRobertson · 2023-01-18T17:42:57Z

synapse/handlers/sync.py

+                have_changed = await self._have_rooms_changed(
+                    sync_result_builder, un_partial_stated_rooms
+                )


Every un-psed room during the sync period is considered to have "changed", i.e., client needs to be informed.

Rooms which are partial stated need to not sure up in a non-lazy sync. Where is this done? Should this be here?

EJ: this method is better called "should_we_tell_the_client_about_this_room_lol"

DMRobertson · 2023-01-18T17:57:19Z

synapse/handlers/sync.py

+                entry = RoomSyncResultBuilder(
+                    room_id=room_id,
+                    rtype="joined",
+                    events=None,


EJ: are we going to lose events from room_events? Maybe... but we should treat this like a newly_joined room (it is from the client's perspective) so I think this is fine

EJ: Does newly_joined_rooms need to be updated to include un-partial-stated rooms and exclude those which are currently partial stated? See the return value of this function

DMRobertson · 2023-01-18T18:00:23Z

synapse/handlers/sync.py

+            # Do not include rooms that we don't have the full state yet
+            # in case of non lazy-loading-members sync.
+            if (
+                not sync_config.filter_collection.lazy_load_members()
+            ) and await self.store.is_partial_state_room(event.room_id):
+                continue
+


In an initial sync, we completely ignore rooms that are currently partially stated

DMRobertson · 2023-01-18T18:42:34Z

synapse/handlers/sync.py

+            since_token
+            and not sync_result_builder.sync_config.filter_collection.lazy_load_members()
+        ):
            un_partial_stated_rooms_since = 0
            if sync_result_builder.since_token is not None:
                un_partial_stated_rooms_since = (


Because we're checking since_token on +1825, we always hit +1830. So 1828 and 1829 are redundant.

MatMaul · 2023-01-18T22:56:20Z

Proper wake up of the long polling syncs has been added, and a matching test has been added to complement.

DMRobertson · 2023-01-20T13:32:51Z

also do we want / need to do anything for rooms that become un-partial stated after we leave them?

@squahtx Putting aside the fact that we can't leave during partial state yet... (oh, I guess you could be kicked)...

We think this should be okay.

Eager syncs won't see the room when we partial-join
Eager syncs won't see anything when we leave during partial-state, because partial state rooms are hidden from eager syncs
When the resync completes, the call to get_un_partial_stated_rooms_between will only return rooms that we're currently joined to (via its third argument).

Worth a complement test though. I'll add it as a todo on the leave-during-resync issue.

DMRobertson · 2023-01-22T21:48:55Z

Superseded by #14870

MatMaul self-assigned this Jan 13, 2023

MatMaul force-pushed the mv/non-ll-sync-fast-join branch 2 times, most recently from 39a0ddc to 6b43520 Compare January 16, 2023 17:06

MatMaul changed the title ~~[WIP] Non lazy loading sync not blocking during fast join~~ Non lazy loading sync not blocking during fast join Jan 16, 2023

MatMaul force-pushed the mv/non-ll-sync-fast-join branch 2 times, most recently from abbfb9d to 13ea6fa Compare January 17, 2023 08:11

Non lazy loading sync not blocking during fast join

3bad02f

Signed-off-by: Mathieu Velten <mathieuv@matrix.org>

MatMaul force-pushed the mv/non-ll-sync-fast-join branch from 13ea6fa to 3bad02f Compare January 17, 2023 08:32

MatMaul marked this pull request as ready for review January 17, 2023 09:59

MatMaul requested a review from a team as a code owner January 17, 2023 09:59

DMRobertson reviewed Jan 17, 2023

View reviewed changes

Mathieu Velten and others added 7 commits January 17, 2023 15:28

Address comments

661e25b

Apply suggestions from code review

b01ec64

Co-authored-by: David Robertson <davidr@element.io>

Move changelog to feature

4441f5b

Use AbstractSet

33d9642

typo un_partial_state vs un_partial_stated

6023bee

Address comment

1cca9eb

oups whitespace

fbfafca

DMRobertson approved these changes Jan 17, 2023

View reviewed changes

MatMaul commented Jan 18, 2023

View reviewed changes

synapse/handlers/sync.py Show resolved Hide resolved

Mathieu Velten added 2 commits January 18, 2023 12:38

Optimize get_un_partial_stated_rooms_between

6472178

Do not call get_un_partial_stated_rooms_between on initial sync

7b60abe

DMRobertson self-requested a review January 18, 2023 14:31

DMRobertson reviewed Jan 18, 2023

View reviewed changes

DMRobertson suggested changes Jan 18, 2023

View reviewed changes

DMRobertson reviewed Jan 18, 2023

View reviewed changes

Notify user stream listeners to wake up long polling syncs

46062d8

DMRobertson mentioned this pull request Jan 19, 2023

Faster joins: omit partial rooms from eager syncs until the resync completes #14870

Merged

DMRobertson mentioned this pull request Jan 20, 2023

Faster joins: Handle leaving a room while re-syncing state #12802

Closed

DMRobertson closed this Jan 22, 2023

This was referenced Feb 14, 2023

Faster joins: Omit device list updates from partial state rooms in /sync #15069

Merged

Check eager sync behaviour after leave then un-partial-stating #15090

Open

matrixbot mentioned this pull request Dec 21, 2023

Check eager sync behaviour after leave then un-partial-stating element-hq/synapse#15090

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Non lazy loading sync not blocking during fast join #14831

Non lazy loading sync not blocking during fast join #14831

MatMaul commented Jan 12, 2023 •

edited by DMRobertson

Loading

DMRobertson left a comment

MatMaul commented Jan 17, 2023 •

edited

Loading

DMRobertson left a comment

squahtx commented Jan 17, 2023 •

edited

Loading

squahtx commented Jan 17, 2023

MatMaul commented Jan 18, 2023

DMRobertson Jan 18, 2023 •

edited

Loading

MatMaul Jan 18, 2023

MatMaul commented Jan 18, 2023 •

edited

Loading

DMRobertson left a comment

DMRobertson Jan 18, 2023

DMRobertson Jan 18, 2023

DMRobertson Jan 18, 2023

DMRobertson Jan 18, 2023

DMRobertson Jan 18, 2023

DMRobertson Jan 18, 2023

DMRobertson Jan 18, 2023

MatMaul commented Jan 18, 2023

DMRobertson commented Jan 20, 2023

DMRobertson commented Jan 22, 2023

Non lazy loading sync not blocking during fast join #14831

Non lazy loading sync not blocking during fast join #14831

Conversation

MatMaul commented Jan 12, 2023 • edited by DMRobertson Loading

Pull Request Checklist

DMRobertson left a comment

Choose a reason for hiding this comment

MatMaul commented Jan 17, 2023 • edited Loading

DMRobertson left a comment

Choose a reason for hiding this comment

squahtx commented Jan 17, 2023 • edited Loading

squahtx commented Jan 17, 2023

MatMaul commented Jan 18, 2023

DMRobertson Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatMaul commented Jan 18, 2023 • edited Loading

DMRobertson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatMaul commented Jan 18, 2023

DMRobertson commented Jan 20, 2023

DMRobertson commented Jan 22, 2023

MatMaul commented Jan 12, 2023 •

edited by DMRobertson

Loading

MatMaul commented Jan 17, 2023 •

edited

Loading

squahtx commented Jan 17, 2023 •

edited

Loading

DMRobertson Jan 18, 2023 •

edited

Loading

MatMaul commented Jan 18, 2023 •

edited

Loading