-
Notifications
You must be signed in to change notification settings - Fork 739
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
Workaround to fetch all the pending toDevice events from a Synapse homeserver #4614
Conversation
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.
I am not sure this will work for the SyncWorker (after a push?)
You might need to schedule a subsequent background sync in this case
…meserver - SyncWorker
automaticallyBackgroundSync(workManagerProvider, params.sessionId, params.timeout, params.delay, forceImmediate = hasToDeviceEvents) | ||
} else if (hasToDeviceEvents) { | ||
// Previous response has toDevice events, request an immediate sync request | ||
requireBackgroundSync(workManagerProvider, params.sessionId, 0) |
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 is not a workaround actually, and we should always do that, even it the bug is fixed on Synapse, WDYT @BillCarsonFr ?
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.
Yes (workaround is to set timeout to 0, which is probably default here ?)
We should check if we try to redecrypt existing notifications after the catchup sync?
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.
Ok, thanks.
We should check if we try to redecrypt existing notifications after the catchup sync?
To be honest I did not do any test, I'm just coding it. Not sure how I can reproduce the test conditions
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.
Just a last comment, I noticed that on iOS when catching up we don't try ti upload new otk or to handle room key request until the catchup sync (to get all to devices) is done https://github.com/matrix-org/matrix-ios-sdk/blob/4e8db1ee5d47e7cb3bcfc4b02508c9f56996fad0/MatrixSDK/Crypto/MXCrypto.m#L931
issue ref: element-hq/element-web#2782
It will wait for the next release (1.3.11). |
LGTM, @BillCarsonFr can you approve this PR please? |
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.
LGTM
Fixes #4612