search

matrix-org / matrix-js-sdk

Matrix Client-Server SDK for JavaScript
Apache License 2.0
1.6k stars 591 forks source link

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

Closed ara4n closed 8 months ago

ara4n commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Should also check for rate limits and use retry_after_ms

BillCarsonFr commented 3 years ago

See https://github.com/matrix-org/matrix-js-sdk/issues/1866

richvdh commented 8 months ago

duplicate of #1866