WIP: Fix rapid empty sync responses #841
No reviewers
Jade
Labels
No labels
Bug
Cherry-picking
Database
Dependencies
Difficulty
Easy
Difficulty
Hard
Difficulty
Medium
Documentation
Enhancement
Good first issue
Help wanted
Inherited
Matrix/Administration
Matrix/Appservices
Matrix/Auth
Matrix/Client
Matrix/Core
Matrix/Federation
Matrix/MSC
Matrix/Media
Meta
Meta/Packaging
Priority
Blocking
Priority
High
Priority
Low
Security
Status
Confirmed
Status
Duplicate
Status
Invalid
Status
Needs Investigation
Wont fix
old/ci/cd
old/rust
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: continuwuation/continuwuity#841
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "nex/fix-eager-empty-sync"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This PR fixes the issue where /sync returns immediately with no valuable data, yet still increments the sync token.
Currently, this fixes the issue where the sync timeout does not default properly. All that needs to be done before this is ready for merge, is preventing the sync token from incrementing (which also coincidentally brings us in line with the Synapse behaviour)
@ -135,3 +135,3 @@let watcher = services.sync.watch(sender_user, sender_device);let response = build_sync_events(&services, &body).await?;trace!("sync body.body.full_state: {}", body.body.full_state);These traces are temporary, probably not necessary now. Remove before merge.
@ -150,3 +156,1 @@let default = Duration::from_secs(30);let duration = cmp::min(body.body.timeout.unwrap_or(default), default);_ = tokio::time::timeout(duration, watcher).await;let default_timeout = Duration::from_secs(30);Set this as a constant at the top of the file if possible
@ -153,0 +157,4 @@let user_timeout = body.body.timeout.unwrap_or(default_timeout);trace!("User timeout: {user_timeout:?}, default timeout: {default_timeout:?}");// If the user default resolves to 0ns or less, use the default timeoutlet timeout = if user_timeout <= Duration::from_secs(0) {Perhaps we also want a minimum timeout? Then doing a min on those values. On the other hand, a timeout of 0 could be legitimate
Was the bug here that not setting a value gave a timeout of 0?
The bug is that
body.body.timeout.unwrap_or(default_timeout)was not using the default timeout, but instead unwrapping to zeroNow that I've slept I've also remembered that a timeout of zero means "return immediately", so technically it is actually valid. I should figure out why the unwrap doesn't work as expected instead of this
@Jade wrote in #841 (comment):
We might want to special-case zero, but have an actual minimum timeout otherwise. Not sure how useful that'll be though
Looks like the fix is a red herring, the issue is actually the timeout simply being skipped as the watcher already claims it has data, when it in fact does not
Yeah that's what I was trying to say last night - Related to #652/files
but the way to 'fix' this is to either use a loop to keep checking or to ensure that we never trigger the watcher unnecessarily. Actual solution is probably a combination of both, with some debugging tools to figure out what's triggering the watcher unnecessarily.
the only non-zero thing in the sync responses is the one-time-key count, is that causing it?
We can't really do a loop here in an efficient manner while also respecting the timeout. When I get some more time, I'll go through and properly trace the watcher
@Aranjedeath wrote in #841 (comment):
No, the issue is this function returning prematurely, essentially. Need to figure out why specifically, since it doesn't actually return anything, meaning it's a bit tricker to see what's caused it to return in the first place
This doesn't fix the issue, a followup PR will. Closing this for now.
Pull request closed