feat(sdk): Sliding sync has a timeout if all lists require a timeout #3853
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
This patch renames tests.
This patch implements and tests `SlidingSyncListLoadingState::is_fully_loaded` for more convenience.
Hywan
requested review from
stefanceriu and
bnjbvr
and removed request for
a team and
stefanceriu
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3853 +/- ##
=======================================
Coverage 84.09% 84.10%
=======================================
Files 262 262
Lines 27646 27657 +11
=======================================
+ Hits 23249 23261 +12
+ Misses 4397 4396 -1 ☔ View full report in Codecov by Sentry. |
Hywan
changed the title
bnjbvr
approved these changes
Aug 19, 2024
There was a problem hiding this comment.
I'd have wished the last 3 commits would be squashed into 59324a6, since they all update the logic introduced in that commit, and having them as separate commits makes the review somewhat harder. LGTM otherwise, assuming my comments will be addressed.
This patch adds a small helper: `SlidingSyncListRequestGenerator::is_selective`.
This patchs adds the `SlidingSyncList::requires_timeout` method to know exactly when a list should trigger a `timeout` on the request.
Hywan
force-pushed
the
feat-sdk-sliding-sync-timeout
branch
from
6f4e062
to
ddc8957
Compare
This patch updates when sliding sync requests have a `timeout`. Prior to this patch, all requests had a `timeout` query, set to the `poll_timeout` duration value. However it means: if there is no data to return, wait `timeout` milliseconds for new data before returning. This definition is correct. Problem: if the current range of a list has no data, the server will wait! It means that, in a situation where there is no update at all, but the client is fetching all rooms batch by batch, it will wait `poll_timeout` for each batch! The behaviour described above is absolutely correct. Some server implementations are less strict though, and we didn't realise our code was doing that, because the server had some optimisations to ignore the timeout if the range wasn't covering all the rooms. Nonetheless, a new server implementation (namely Synapse) is strict, and it confirms we have a bug here. This patch then configures a `timeout` if all lists require a timeout, otherwise there is no `timeout`, which is equivalent to `timeout=0`.
This patch is a small cleanup.
Hywan
force-pushed
the
feat-sdk-sliding-sync-timeout
branch
from
ddc8957
to
a84c172
Compare
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.
This patch updates when sliding sync requests have a
timeout.Prior to this patch, all requests had a
timeoutquery, set to thepoll_timeoutduration value. However it means: if there is no datato return, wait
timeoutmilliseconds for new data before returning.This definition is correct. Problem: if the current range of a list
has no data, the server will wait! It means that, in a situation where
there is no update at all, but the client is fetching all rooms batch by
batch, it will wait
poll_timeoutfor each batch!The behaviour described above is absolutely correct. Some server
implementations are less strict though, and we didn't realise our code
was doing that, because the server had some optimisations to ignore the
timeout if the range wasn't covering all the rooms. Nonetheless, a new
server implementation (namely Synapse) is strict, and it confirms we
have a bug here.
This patch then configures a
timeoutif all lists require atimeout,otherwise there is no
timeout, which is equivalent totimeout=0.