refactor megolm encryption to improve perceived speed #1252
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
- allow applications to pre-send decryption keys before the message is sent - establish new olm sessions with a shorter timeout first, and then re-try in the background with a longer timeout without blocking message sending
foldleft
approved these changes
Mar 10, 2020
| // encryptMessage retries senders in the background before giving | ||
| // up and telling them that there's no olm channel, so we need to | ||
| // wait a bit before checking that we got the message | ||
| setTimeout(resolve, 100); |
There was a problem hiding this comment.
Do we use the jest.useFakeTImers() API?
https://jestjs.io/docs/en/timer-mocks
There was a problem hiding this comment.
I think we have some things that use jest timer mocks, but I'm not sure how to use them in the context of waiting for a background task to complete. Though looking at it again, I see a way of doing it without the timer at all.
| // already have a session for, and share keys with them. Use a | ||
| // shorter timeout when fetching one-time keys. | ||
| await this._shareKeyWithDevices( | ||
| session, key, payload, devicesWithoutSession, errorDevices, 2000, |
There was a problem hiding this comment.
I'm not sure that it's worth creating a constant for. 🤷♂️
src/crypto/algorithms/megolm.js
Outdated
| * @param {module:models/room} room the room the event is in | ||
| */ | ||
| MegolmEncryption.prototype.prepareToEncrypt = function(room) { | ||
| logger.log(`Preparing to encrypt events for ${this._roomId}`); |
There was a problem hiding this comment.
Is this for every message sent? Could get spammy, maybe logger.debug?
|
|
||
| if (this.encryptionPreparation) { | ||
| // We're already preparing something, so don't do anything else. | ||
| // FIXME: check if we need to restart |
There was a problem hiding this comment.
Ideally, file a task and link here
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.
fixes element-hq/element-web#11836