Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from
Nov 8, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7b998dc
Refactor feeding of events to widgets
robintown Nov 4, 2024
ef55f1d
Feed events to widgets as they are decrypted (even if out of order)
robintown Nov 4, 2024
6a0c357
Add braces to if
robintown Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
157 changes: 94 additions & 63 deletions src/stores/widgets/StopGapWidget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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);
};

Expand All @@ -480,72 +481,102 @@ 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 {
robintown marked this conversation as resolved.
Show resolved Hide resolved
const raw = ev.getEffectiveEvent();
this.messaging.feedEvent(raw as IRoomEvent, this.eventListenerRoomId!).catch((e) => {
logger.error("Error sending event to widget: ", e);
});
}
}
}
}
49 changes: 48 additions & 1 deletion test/unit-tests/stores/widgets/StopGapWidget-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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,
Expand Down
Loading