Refactor MatrixClient.encryptAndSendEvent (#4031)

* Replace `pendingEventEncryption` with a Set

We don't actually need the promise, so no need to save it.

This also fixes a resource leak, where we would leak a Promise and a HashMap
entry on each encrypted event.

* Convert `encryptEventIfNeeded` to async function

This means that it will always return a promise, so `encryptAndSendEvent` can't
tell if we are actually encrypting or not. Hence, also move the
`updatePendingEventStatus` into `encryptEventIfNeeded`.

* Simplify `encryptAndSendEvent`

Rewrite this as async.

* Factor out `MatrixClient.shouldEncryptEventForRoom`

* Inline a call to `isRoomEncrypted`

I want to deprecate this thing

spec/unit/matrix-client.spec.ts

@@ -65,7 +65,7 @@ import {
     PolicyScope,
 } from "../../src/models/invites-ignorer";
 import { IOlmDevice } from "../../src/crypto/algorithms/megolm";
-import { QueryDict } from "../../src/utils";
+import { defer, QueryDict } from "../../src/utils";
 import { SyncState } from "../../src/sync";
 import * as featureUtils from "../../src/feature";
 import { StubStore } from "../../src/store/stub";
@@ -1453,6 +1453,8 @@ describe("MatrixClient", function () {
             hasEncryptionStateEvent: jest.fn().mockReturnValue(true),
         } as unknown as Room;
 
+        let mockCrypto: Mocked<Crypto>;
+
         let event: MatrixEvent;
         beforeEach(async () => {
             event = new MatrixEvent({
@@ -1467,11 +1469,12 @@ describe("MatrixClient", function () {
                 expect(getRoomId).toEqual(roomId);
                 return mockRoom;
             };
-            client.crypto = client["cryptoBackend"] = {
+
-                // mock crypto
+            mockCrypto = {
-                encryptEvent: () => new Promise(() => {}),
+                encryptEvent: jest.fn(),
                 stop: jest.fn(),
-            } as unknown as Crypto;
+            } as unknown as Mocked<Crypto>;
+            client.crypto = client["cryptoBackend"] = mockCrypto;
         });
 
         function assertCancelled() {
@@ -1488,12 +1491,21 @@ describe("MatrixClient", function () {
         });
 
         it("should cancel an event which is encrypting", async () => {
+            const encryptEventDefer = defer();
+            mockCrypto.encryptEvent.mockReturnValue(encryptEventDefer.promise);
+
+            const statusPromise = testUtils.emitPromise(event, "Event.status");
             // @ts-ignore protected method access
-            client.encryptAndSendEvent(mockRoom, event);
+            const encryptAndSendPromise = client.encryptAndSendEvent(mockRoom, event);
-            await testUtils.emitPromise(event, "Event.status");
+            await statusPromise;
             expect(event.status).toBe(EventStatus.ENCRYPTING);
             client.cancelPendingEvent(event);
             assertCancelled();
+
+            // now let the encryption complete, and check that the message is not sent.
+            encryptEventDefer.resolve();
+            await encryptAndSendPromise;
+            assertCancelled();
         });
 
         it("should cancel an event which is not sent", () => {

spec/unit/queueToDevice.spec.ts

@@ -265,6 +265,7 @@ describe.each([[StoreType.Memory], [StoreType.IndexedDB]])("queueToDevice (%s st
         });
         const mockRoom = {
             updatePendingEvent: jest.fn(),
+            hasEncryptionStateEvent: jest.fn().mockReturnValue(false),
         } as unknown as Room;
         client.resendEvent(dummyEvent, mockRoom);
 

src/client.ts

@@ -1305,7 +1305,13 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
     protected txnCtr = 0;
     protected mediaHandler = new MediaHandler(this);
     protected sessionId: string;
-    protected pendingEventEncryption = new Map<string, Promise<void>>();
+
+    /** IDs of events which are currently being encrypted.
+     *
+     * This is part of the cancellation mechanism: if the event is no longer listed here when encryption completes,
+     * that tells us that it has been cancelled, and we should not send it.
+     */
+    private eventsBeingEncrypted = new Set<string>();
 
     private useE2eForGroupCall = true;
     private toDeviceMessageQueue: ToDeviceMessageQueue;
@@ -4448,9 +4454,10 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
             throw new Error("cannot cancel an event with status " + event.status);
         }
 
-        // if the event is currently being encrypted then
+        // If the event is currently being encrypted then remove it from the pending list, to indicate that it should
+        // not be sent.
         if (event.status === EventStatus.ENCRYPTING) {
-            this.pendingEventEncryption.delete(event.getId()!);
+            this.eventsBeingEncrypted.delete(event.getId()!);
         } else if (this.scheduler && event.status === EventStatus.QUEUED) {
             // tell the scheduler to forget about it, if it's queued
             this.scheduler.removeEventFromQueue(event);
@@ -4749,29 +4756,27 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      * encrypts the event if necessary; adds the event to the queue, or sends it; marks the event as sent/unsent
      * @returns returns a promise which resolves with the result of the send request
      */
-    protected encryptAndSendEvent(room: Room | null, event: MatrixEvent): Promise<ISendEventResponse> {
+    protected async encryptAndSendEvent(room: Room | null, event: MatrixEvent): Promise<ISendEventResponse> {
-        let cancelled = false;
+        try {
-        // Add an extra Promise.resolve() to turn synchronous exceptions into promise rejections,
+            let cancelled: boolean;
-        // so that we can handle synchronous and asynchronous exceptions with the
+            this.eventsBeingEncrypted.add(event.getId()!);
-        // same code path.
+            try {
-        return Promise.resolve()
+                await this.encryptEventIfNeeded(event, room ?? undefined);
-            .then(() => {
+            } finally {
-                const encryptionPromise = this.encryptEventIfNeeded(event, room ?? undefined);
+                cancelled = !this.eventsBeingEncrypted.delete(event.getId()!);
-                if (!encryptionPromise) return null; // doesn't need encryption
-
-                this.pendingEventEncryption.set(event.getId()!, encryptionPromise);
-                this.updatePendingEventStatus(room, event, EventStatus.ENCRYPTING);
-                return encryptionPromise.then(() => {
-                    if (!this.pendingEventEncryption.has(event.getId()!)) {
-                        // cancelled via MatrixClient::cancelPendingEvent
-                        cancelled = true;
-                        return;
             }
+
+            if (cancelled) {
+                // cancelled via MatrixClient::cancelPendingEvent
+                return {} as ISendEventResponse;
+            }
+
+            // encryptEventIfNeeded may have updated the status from SENDING to ENCRYPTING. If so, we need
+            // to put it back.
+            if (event.status === EventStatus.ENCRYPTING) {
                 this.updatePendingEventStatus(room, event, EventStatus.SENDING);
-                });
+            }
-            })
+
-            .then(() => {
-                if (cancelled) return {} as ISendEventResponse;
             let promise: Promise<ISendEventResponse> | null = null;
             if (this.scheduler) {
                 // if this returns a promise then the scheduler has control now and will
@@ -4796,49 +4801,57 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
                 }
             }
 
-                return promise;
+            return await promise;
-            })
+        } catch (err) {
-            .catch((err) => {
+            this.logger.error("Error sending event", err);
-                this.logger.error("Error sending event", err.stack || err);
             try {
                 // set the error on the event before we update the status:
                 // updating the status emits the event, so the state should be
                 // consistent at that point.
-                    event.error = err;
+                event.error = <MatrixError>err;
                 this.updatePendingEventStatus(room, event, EventStatus.NOT_SENT);
             } catch (e) {
-                    this.logger.error("Exception in error handler!", (<Error>e).stack || err);
+                this.logger.error("Exception in error handler!", e);
             }
             if (err instanceof MatrixError) {
                 err.event = event;
             }
             throw err;
-            });
+        }
     }
 
-    private encryptEventIfNeeded(event: MatrixEvent, room?: Room): Promise<void> | null {
+    private async encryptEventIfNeeded(event: MatrixEvent, room?: Room): Promise<void> {
-        if (event.isEncrypted()) {
+        // If the room is unknown, we cannot encrypt for it
-            // this event has already been encrypted; this happens if the
+        if (!room) return;
-            // encryption step succeeded, but the send step failed on the first
-            // attempt.
-            return null;
-        }
 
-        if (event.isRedaction()) {
+        if (!this.shouldEncryptEventForRoom(event, room)) return;
-            // Redactions do not support encryption in the spec at this time,
-            // whilst it mostly worked in some clients, it wasn't compliant.
-            return null;
-        }
-
-        if (!room || !this.isRoomEncrypted(event.getRoomId()!)) {
-            return null;
-        }
 
         if (!this.cryptoBackend && this.usingExternalCrypto) {
             // The client has opted to allow sending messages to encrypted
             // rooms even if the room is encrypted, and we haven't set up
             // crypto. This is useful for users of matrix-org/pantalaimon
-            return null;
+            return;
+        }
+
+        if (!this.cryptoBackend) {
+            throw new Error("This room is configured to use encryption, but your client does not support encryption.");
+        }
+
+        this.updatePendingEventStatus(room, event, EventStatus.ENCRYPTING);
+        await this.cryptoBackend.encryptEvent(event, room);
+    }
+
+    /**
+     * Determine whether a given event should be encrypted when we send it to the given room.
+     *
+     * This takes into account event type and room configuration.
+     */
+    private shouldEncryptEventForRoom(event: MatrixEvent, room: Room): boolean {
+        if (event.isEncrypted()) {
+            // this event has already been encrypted; this happens if the
+            // encryption step succeeded, but the send step failed on the first
+            // attempt.
+            return false;
         }
 
         if (event.getType() === EventType.Reaction) {
@@ -4852,14 +4865,23 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
             // The reaction key / content / emoji value does warrant encrypting, but
             // this will be handled separately by encrypting just this value.
             // See https://github.com/matrix-org/matrix-doc/pull/1849#pullrequestreview-248763642
-            return null;
+            return false;
         }
 
-        if (!this.cryptoBackend) {
+        if (event.isRedaction()) {
-            throw new Error("This room is configured to use encryption, but your client does not support encryption.");
+            // Redactions do not support encryption in the spec at this time.
+            // Whilst it mostly worked in some clients, it wasn't compliant.
+            return false;
         }
 
-        return this.cryptoBackend.encryptEvent(event, room);
+        // If the room has an m.room.encryption event, we should encrypt.
+        if (room.hasEncryptionStateEvent()) return true;
+
+        // If we have a crypto impl, and *it* thinks we should encrypt, then we should.
+        if (this.crypto?.isRoomEncrypted(room.roomId)) return true;
+
+        // Otherwise, no need to encrypt.
+        return false;
     }
 
     /**

src/models/event.ts

@@ -350,7 +350,7 @@ export class MatrixEvent extends TypedEventEmitter<MatrixEventEmittedEvents, Mat
     /**
      * most recent error associated with sending the event, if any
      * @privateRemarks
-     * Should be read-only
+     * Should be read-only. May not be a MatrixError.
      */
     public error: MatrixError | null = null;
     /**