Skip to content

Commit 7ba71f7

Browse files
committed
Make sure that MegolmEncryption.setupPromise always resolves
The ensureOutboundSession users and modifies the setupPromise of the MegolmEncryption class. Some comments suggest that setupPromise will always resolve, in other words it should never contain a promise that will get rejected. Other comments also seem to suggest that the return value of ensureOutboundSession, a promise as well, may fail. The critical error here is that the promise that gets set as the next setupPromise, as well as the promise that ensureOutboundSession returns, is the same promise. It seems that the intention was for setupPromise to contain a promise that will always resolve to either `null` or `OutboundSessionInfo`. We can see that a couple of lines before we set setupPromise to its new value we construct a promise that logs and discards errors using the `Promise.catch()` method. The `Promise.catch()` method does not mutate the promise, instead it returns a new promise. The intention of the original author might have been to set the next setupPromise to the promise which `Promise.catch()` produces. This patch modifies the updating of setupPromise in the ensureOutboundSession so that setupPromise discards errors correctly. Using `>>=` to represent the promise chaining operation, setupPromise is now updated using the following logic: setupPromise = previousSetupPromise >>= setup >>= discardErrors
1 parent 77d6def commit 7ba71f7

File tree

1 file changed

+22
-5
lines changed

1 file changed

+22
-5
lines changed

src/crypto/algorithms/megolm.ts

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,21 @@ class MegolmEncryption extends EncryptionAlgorithm {
265265
* @param {boolean} [singleOlmCreationPhase] Only perform one round of olm
266266
* session creation
267267
*
268+
* This method updates the setupPromise field of the class by chaining a new
269+
* call on top of the existing promise, and then catching and discarding any
270+
* errors that might happen while setting up the outbound group session. This
271+
* is done to ensure that `setupPromise` always resolves to `null` or the
272+
* `OutboundSessionInfo`.
273+
*
274+
* Using `>>=` to represent the promise chaining operation, it does the
275+
* following:
276+
*
277+
* setupPromise = previousSetupPromise >>= setup >>= discardErrors
278+
*
279+
* The initial value for the `setupPromise` is a promise that resolves to
280+
* `null`. The forceDiscardSession() resets setupPromise to this initial
281+
* promise.
282+
*
268283
* @return {Promise} Promise which resolves to the
269284
* OutboundSessionInfo when setup is complete.
270285
*/
@@ -295,18 +310,20 @@ class MegolmEncryption extends EncryptionAlgorithm {
295310
};
296311

297312
// first wait for the previous share to complete
298-
const prom = this.setupPromise.then(setup);
313+
const fallible = this.setupPromise.then(setup);
299314

300315
// Ensure any failures are logged for debugging
301-
prom.catch(e => {
316+
const infallible = fallible.catch(e => {
302317
logger.error(`Failed to setup outbound session in ${this.roomId}`, e);
318+
return null;
303319
});
304320

305-
// setupPromise resolves to `session` whether or not the share succeeds
306-
this.setupPromise = prom;
321+
// setupPromise resolves to `null` or the `OutboundSessionInfo` whether
322+
// or not the share succeeds
323+
this.setupPromise = infallible;
307324

308325
// but we return a promise which only resolves if the share was successful.
309-
return prom;
326+
return fallible;
310327
}
311328

312329
private async prepareSession(

0 commit comments

Comments
 (0)