Skip to content
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

Sending to-device messages looks incredibly fragile, causing UISIs #1990

Closed
ara4n opened this issue Oct 20, 2021 · 4 comments
Closed

Sending to-device messages looks incredibly fragile, causing UISIs #1990

ara4n opened this issue Oct 20, 2021 · 4 comments

Comments

@ara4n
Copy link
Member

ara4n commented Oct 20, 2021

mildly terrified that matrix-js-sdk calls this.baseApis.sendToDevice("m.room.encrypted") 7 times
but sendToDevice has no retry mechanism
but almost none of those calls to sendToDevice appear to do retries
so if the PUT fails... the whole thing fails, whether that's keysharing or re-sharing or requesting or... anything.

That said, encryptAndSendKeysToDevices() has uses Promise.all().then() to send its messages... so if any of the sends fail, the whole attempt to encrypt will fail, so we will re-send those messages if it ever retries.

@turt2live
Copy link
Member

A fix for the function would be:

const promises = [];
for (const userId of userIds) {
    // do not await - we want these to run concurrently
    promises.push(this.encryptAndSend(userId, props...).then(() => markSent()).catch(e => retry())); // don't let `retry()` throw
}
await Promise.all(promises);
// continue event sending

this is a lot easier on paper, though.

@BillCarsonFr
Copy link
Member

Should also check for rate limits and use retry_after_ms

@BillCarsonFr
Copy link
Member

See #1866

@richvdh
Copy link
Member

richvdh commented Feb 16, 2024

duplicate of #1866

@richvdh richvdh closed this as completed Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants