Feed events to widgets as they are decrypted (even if out of order) (#28376)

robintown · web-flow · commit d0cddc5b6626 · 2024-11-08T14:35:47.000Z
* Refactor feeding of events to widgets

This is a pure refactor with (hopefully) no behavior changes.

* Feed events to widgets as they are decrypted (even if out of order)

The code that feeds events to widgets tries to enforce that only events from the end of the timeline will be passed through. This is to prevent old, irrelevant events from being passed to widgets as the timeline is back-filled. However, since encrypted events need to be decrypted asynchronously, it's not possible to feed them to a widget in a strictly linear order without introducing some kind of blocking or unreliable delivery. This code has been dropping events when they're decrypted out of order, which we consider to be an undesirable behavior.

The solution provided here is that, to reflect the asynchronous nature of decryption, encrypted events that arrive at the end of the timeline will be fed to a widget whenever they finish decrypting, even if this means feeding them out of order. For now we're not aware of any widgets that care about knowing the exact order of events in the timeline, but if such a need reveals itself later, we can explore adding ordering information to this part of the widget API.

* Add braces to if
diff --git a/src/stores/widgets/StopGapWidget.ts b/src/stores/widgets/StopGapWidget.ts
@@ -154,7 +154,10 @@ export class StopGapWidget extends EventEmitter {
     private kind: WidgetKind;
     private readonly virtual: boolean;
     private readUpToMap: { [roomId: string]: string } = {}; // room ID to event ID
-    private stickyPromise?: () => Promise<void>; // This promise will be called and needs to resolve before the widget will actually become sticky.
+    // This promise will be called and needs to resolve before the widget will actually become sticky.
+    private stickyPromise?: () => Promise<void>;
+    // Holds events that should be fed to the widget once they finish decrypting
+    private readonly eventsToFeed = new WeakSet<MatrixEvent>();
 
     public constructor(private appTileProps: IAppTileProps) {
         super();
@@ -465,12 +468,10 @@ export class StopGapWidget extends EventEmitter {
 
     private onEvent = (ev: MatrixEvent): void => {
         this.client.decryptEventIfNeeded(ev);
-        if (ev.isBeingDecrypted() || ev.isDecryptionFailure()) return;
         this.feedEvent(ev);
     };
 
     private onEventDecrypted = (ev: MatrixEvent): void => {
-        if (ev.isDecryptionFailure()) return;
         this.feedEvent(ev);
     };
 
@@ -480,72 +481,103 @@ export class StopGapWidget extends EventEmitter {
         await this.messaging?.feedToDevice(ev.getEffectiveEvent() as IRoomEvent, ev.isEncrypted());
     };
 
-    private feedEvent(ev: MatrixEvent): void {
-        if (!this.messaging) return;
-
-        // Check to see if this event would be before or after our "read up to" marker. If it's
-        // before, or we can't decide, then we assume the widget will have already seen the event.
-        // If the event is after, or we don't have a marker for the room, then we'll send it through.
-        //
-        // This approach of "read up to" prevents widgets receiving decryption spam from startup or
-        // receiving out-of-order events from backfill and such.
-        //
-        // Skip marker timeline check for events with relations to unknown parent because these
-        // events are not added to the timeline here and will be ignored otherwise:
-        // https://github.com/matrix-org/matrix-js-sdk/blob/d3dfcd924201d71b434af3d77343b5229b6ed75e/src/models/room.ts#L2207-L2213
-        let isRelationToUnknown: boolean | undefined = undefined;
-        const upToEventId = this.readUpToMap[ev.getRoomId()!];
-        if (upToEventId) {
-            // Small optimization for exact match (prevent search)
-            if (upToEventId === ev.getId()) {
-                return;
-            }
+    /**
+     * Determines whether the event has a relation to an unknown parent.
+     */
+    private relatesToUnknown(ev: MatrixEvent): boolean {
+        // Replies to unknown events don't count
+        if (!ev.relationEventId || ev.replyEventId) return false;
+        const room = this.client.getRoom(ev.getRoomId());
+        return room === null || !room.findEventById(ev.relationEventId);
+    }
 
-            // should be true to forward the event to the widget
-            let shouldForward = false;
-
-            const room = this.client.getRoom(ev.getRoomId()!);
-            if (!room) return;
-            // Timelines are most recent last, so reverse the order and limit ourselves to 100 events
-            // to avoid overusing the CPU.
-            const timeline = room.getLiveTimeline();
-            const events = arrayFastClone(timeline.getEvents()).reverse().slice(0, 100);
-
-            for (const timelineEvent of events) {
-                if (timelineEvent.getId() === upToEventId) {
-                    break;
-                } else if (timelineEvent.getId() === ev.getId()) {
-                    shouldForward = true;
-                    break;
-                }
-            }
+    /**
+     * Determines whether the event comes from a room that we've been invited to
+     * (in which case we likely don't have the full timeline).
+     */
+    private isFromInvite(ev: MatrixEvent): boolean {
+        const room = this.client.getRoom(ev.getRoomId());
+        return room?.getMyMembership() === KnownMembership.Invite;
+    }
 
-            if (!shouldForward) {
-                // checks that the event has a relation to unknown event
-                isRelationToUnknown =
-                    !ev.replyEventId && !!ev.relationEventId && !room.findEventById(ev.relationEventId);
-                if (!isRelationToUnknown) {
-                    // Ignore the event: it is before our interest.
-                    return;
-                }
-            }
+    /**
+     * Advances the "read up to" marker for a room to a certain event. No-ops if
+     * the event is before the marker.
+     * @returns Whether the "read up to" marker was advanced.
+     */
+    private advanceReadUpToMarker(ev: MatrixEvent): boolean {
+        const evId = ev.getId();
+        if (evId === undefined) return false;
+        const roomId = ev.getRoomId();
+        if (roomId === undefined) return false;
+        const room = this.client.getRoom(roomId);
+        if (room === null) return false;
+
+        const upToEventId = this.readUpToMap[ev.getRoomId()!];
+        if (!upToEventId) {
+            // There's no marker yet; start it at this event
+            this.readUpToMap[roomId] = evId;
+            return true;
         }
 
-        // Skip marker assignment if membership is 'invite', otherwise 'm.room.member' from
-        // invitation room will assign it and new state events will be not forwarded to the widget
-        // because of empty timeline for invitation room and assigned marker.
-        const evRoomId = ev.getRoomId();
-        const evId = ev.getId();
-        if (evRoomId && evId) {
-            const room = this.client.getRoom(evRoomId);
-            if (room && room.getMyMembership() === KnownMembership.Join && !isRelationToUnknown) {
-                this.readUpToMap[evRoomId] = evId;
+        // Small optimization for exact match (skip the search)
+        if (upToEventId === evId) return false;
+
+        // Timelines are most recent last, so reverse the order and limit ourselves to 100 events
+        // to avoid overusing the CPU.
+        const timeline = room.getLiveTimeline();
+        const events = arrayFastClone(timeline.getEvents()).reverse().slice(0, 100);
+
+        for (const timelineEvent of events) {
+            if (timelineEvent.getId() === upToEventId) {
+                // The event must be somewhere before the "read up to" marker
+                return false;
+            } else if (timelineEvent.getId() === ev.getId()) {
+                // The event is after the marker; advance it
+                this.readUpToMap[roomId] = evId;
+                return true;
             }
         }
 
-        const raw = ev.getEffectiveEvent();
-        this.messaging.feedEvent(raw as IRoomEvent, this.eventListenerRoomId!).catch((e) => {
-            logger.error("Error sending event to widget: ", e);
-        });
+        // We can't say for sure whether the widget has seen the event; let's
+        // just assume that it has
+        return false;
+    }
+
+    private feedEvent(ev: MatrixEvent): void {
+        if (this.messaging === null) return;
+        if (
+            // If we had decided earlier to feed this event to the widget, but
+            // it just wasn't ready, give it another try
+            this.eventsToFeed.delete(ev) ||
+            // Skip marker timeline check for events with relations to unknown parent because these
+            // events are not added to the timeline here and will be ignored otherwise:
+            // https://github.com/matrix-org/matrix-js-sdk/blob/d3dfcd924201d71b434af3d77343b5229b6ed75e/src/models/room.ts#L2207-L2213
+            this.relatesToUnknown(ev) ||
+            // Skip marker timeline check for rooms where membership is
+            // 'invite', otherwise the membership event from the invitation room
+            // will advance the marker and new state events will not be
+            // forwarded to the widget.
+            this.isFromInvite(ev) ||
+            // Check whether this event would be before or after our "read up to" marker. If it's
+            // before, or we can't decide, then we assume the widget will have already seen the event.
+            // If the event is after, or we don't have a marker for the room, then the marker will advance and we'll
+            // send it through.
+            // This approach of "read up to" prevents widgets receiving decryption spam from startup or
+            // receiving ancient events from backfill and such.
+            this.advanceReadUpToMarker(ev)
+        ) {
+            // If the event is still being decrypted, remember that we want to
+            // feed it to the widget (even if not strictly in the order given by
+            // the timeline) and get back to it later
+            if (ev.isBeingDecrypted() || ev.isDecryptionFailure()) {
+                this.eventsToFeed.add(ev);
+            } else {
+                const raw = ev.getEffectiveEvent();
+                this.messaging.feedEvent(raw as IRoomEvent, this.eventListenerRoomId!).catch((e) => {
+                    logger.error("Error sending event to widget: ", e);
+                });
+            }
+        }
     }
 }
diff --git a/test/unit-tests/stores/widgets/StopGapWidget-test.ts b/test/unit-tests/stores/widgets/StopGapWidget-test.ts
@@ -8,7 +8,14 @@ Please see LICENSE files in the repository root for full details.
 
 import { mocked, MockedObject } from "jest-mock";
 import { last } from "lodash";
-import { MatrixEvent, MatrixClient, ClientEvent, EventTimeline } from "matrix-js-sdk/src/matrix";
+import {
+    MatrixEvent,
+    MatrixClient,
+    ClientEvent,
+    EventTimeline,
+    EventType,
+    MatrixEventEvent,
+} from "matrix-js-sdk/src/matrix";
 import { ClientWidgetApi, WidgetApiFromWidgetAction } from "matrix-widget-api";
 import { waitFor } from "jest-matrix-react";
 
@@ -134,6 +141,46 @@ describe("StopGapWidget", () => {
             expect(messaging.feedEvent).toHaveBeenLastCalledWith(event2.getEffectiveEvent(), "!1:example.org");
         });
 
+        it("feeds decrypted events asynchronously", async () => {
+            const event1Encrypted = new MatrixEvent({
+                event_id: event1.getId(),
+                type: EventType.RoomMessageEncrypted,
+                sender: event1.sender?.userId,
+                room_id: event1.getRoomId(),
+                content: {},
+            });
+            const decryptingSpy1 = jest.spyOn(event1Encrypted, "isBeingDecrypted").mockReturnValue(true);
+            client.emit(ClientEvent.Event, event1Encrypted);
+            const event2Encrypted = new MatrixEvent({
+                event_id: event2.getId(),
+                type: EventType.RoomMessageEncrypted,
+                sender: event2.sender?.userId,
+                room_id: event2.getRoomId(),
+                content: {},
+            });
+            const decryptingSpy2 = jest.spyOn(event2Encrypted, "isBeingDecrypted").mockReturnValue(true);
+            client.emit(ClientEvent.Event, event2Encrypted);
+            expect(messaging.feedEvent).not.toHaveBeenCalled();
+
+            // "Decrypt" the events, but in reverse order; first event 2…
+            event2Encrypted.event.type = event2.getType();
+            event2Encrypted.event.content = event2.getContent();
+            decryptingSpy2.mockReturnValue(false);
+            client.emit(MatrixEventEvent.Decrypted, event2Encrypted);
+            expect(messaging.feedEvent).toHaveBeenCalledTimes(1);
+            expect(messaging.feedEvent).toHaveBeenLastCalledWith(event2Encrypted.getEffectiveEvent(), "!1:example.org");
+            // …then event 1
+            event1Encrypted.event.type = event1.getType();
+            event1Encrypted.event.content = event1.getContent();
+            decryptingSpy1.mockReturnValue(false);
+            client.emit(MatrixEventEvent.Decrypted, event1Encrypted);
+            // The events should be fed in that same order so that event 2
+            // doesn't have to be blocked on the decryption of event 1 (or
+            // worse, dropped)
+            expect(messaging.feedEvent).toHaveBeenCalledTimes(2);
+            expect(messaging.feedEvent).toHaveBeenLastCalledWith(event1Encrypted.getEffectiveEvent(), "!1:example.org");
+        });
+
         it("should not feed incoming event if not in timeline", () => {
             const event = mkEvent({
                 event: true,
