Activate share_pos on the room-list sliding sync instance #4035
+152
−9
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MatMaul
force-pushed
the
share-pos-room-list
branch
from
3e42b53
to
d17e83f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4035 +/- ##
==========================================
- Coverage 84.90% 84.86% -0.04%
==========================================
Files 271 271
Lines 29070 29071 +1
==========================================
- Hits 24681 24671 -10
- Misses 4389 4400 +11 ☔ View full report in Codecov by Sentry. |
MatMaul
force-pushed
the
share-pos-room-list
branch
from
d17e83f
to
07310ee
Compare
|
Ahhh crypto dragons has been triggered :/ . Time to set up complement-crypto :) |
MatMaul
force-pushed
the
share-pos-room-list
branch
from
07310ee
to
11d63f2
Compare
|
Hum |
MatMaul
force-pushed
the
share-pos-room-list
branch
from
11d63f2
to
4140bbd
Compare
|
The failed test was a timeout so I think it's a false alarm. |
bnjbvr
requested changes
Sep 30, 2024
|
It failed again, it may not be a flake especially since it's restart related ? |
|
The log files show: ..which imply that |
|
A successful local run shows: |
|
Right, found the cause. When it starts up, 2 sliding sync connections are made: This causes 2x HTTP requests to /sync. On passing tests, both finish quickly. On failing tests, one does not return at all for the duration of the test. Looking at the MITM dump for the failed run and it's clear that the one that doesn't return is {
"conn_id": "room-list",
"extensions": {
"account_data": {
"enabled": true
},
"receipts": {
"enabled": true,
"rooms": [
"*"
]
},
"typing": {
"enabled": true
}
},
"lists": {
"all_rooms": {
"filters": {
"not_room_types": [
"m.space"
]
},
"include_heroes": true,
"ranges": [
[
0,
0
]
],
"required_state": [
[
"m.room.encryption",
""
],
[
"m.room.member",
"$LAZY"
],
[
"m.room.member",
"$ME"
],
[
"m.room.name",
""
],
[
"m.room.canonical_alias",
""
],
[
"m.room.power_levels",
""
]
],
"timeline_limit": 1
}
},
"txn_id": "86a22ed3a03948dc985c7c32be1b5327"
}I suspect the sync code assumes that room-list requests return immediately on startup (which has been true up until now). By sharing the So this is a real bug unfortunately. The reason why it is flakey is likely due to the slow speed of GHA boxes. I suspect the test itself runs much more slowly, giving enough time for the new data to come down before the restart, meaning when it starts up again there is no new data to fetch, hence it blocks waiting for more data. The end-user impact of this will presumably be a spinner showing for 30s (as it's also waiting on the same listener the tests are) upon restart. You likely didn't see this because your account is way more active, meaning there is a high chance something will come down before the timeout, but in a test scenario this is not the case. |
|
@kegsay thanks a lot for the detailed investigation 🙏 The fix should be easy-ish with all those infos, I'll have a look tomorrow. |
|
The fix fwiw is to just use ?timeout=0 on the first sync (before the room list state is loaded). This matches sync V2 behaviour in other clients. |
I don't think it will be useful: syncv2 spec is saying that no timeout specified is the same as timeout=0. Synapse code seems to agree, for both syncv2 and SSS. |
|
Nevermind, timeout of the first req is 60s in your logs. |
It seems we don't really need to use this trick here, cf 492166a. |
Hywan
requested changes
Oct 30, 2024
| @@ -156,6 +156,9 @@ impl RoomListService { | |||
| .with_to_device_extension( | |||
| assign!(http::request::ToDevice::default(), { enabled: Some(true) }), | |||
| ); | |||
| } else { | |||
| // TODO: This is racy with encryption, needs cross-process lock | |||
There was a problem hiding this comment.
Please update the comment to specify that this is racy if encryption is used within the room list.
| pub fn loading_state(&self) -> Subscriber<RoomListLoadingState> { | ||
| self.loading_state.subscribe() | ||
| self.loading_state.subscribe_reset() |
|
We are very close to see a merge here. As soon as you address the last comments, we are good. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #3936.
Signed-off-by: Mathieu Velten [email protected]