From cd7c519dc435af4b8cf4600e03a4e58706d789d6 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Fri, 7 Jul 2023 09:48:09 +0100 Subject: [PATCH] Prevent threads code from making identical simultaneous API hits (#3541) --- .../matrix-client-event-timeline.spec.ts | 107 +----------------- spec/test-utils/thread.ts | 2 +- spec/unit/models/thread.spec.ts | 12 +- spec/unit/room.spec.ts | 2 +- src/models/thread.ts | 55 +++++++-- 5 files changed, 58 insertions(+), 120 deletions(-) diff --git a/spec/integ/matrix-client-event-timeline.spec.ts b/spec/integ/matrix-client-event-timeline.spec.ts index cba141f988a..9a8f7b3058f 100644 --- a/spec/integ/matrix-client-event-timeline.spec.ts +++ b/spec/integ/matrix-client-event-timeline.spec.ts @@ -598,12 +598,6 @@ describe("MatrixClient event timelines", function () { await client.stopClient(); // we don't need the client to be syncing at this time const room = client.getRoom(roomId)!; - httpBackend - .when("GET", "/rooms/!foo%3Abar/event/" + encodeURIComponent(THREAD_ROOT.event_id!)) - .respond(200, function () { - return THREAD_ROOT; - }); - httpBackend .when("GET", "/rooms/!foo%3Abar/event/" + encodeURIComponent(THREAD_ROOT.event_id!)) .respond(200, function () { @@ -634,12 +628,6 @@ describe("MatrixClient event timelines", function () { const thread = room.createThread(THREAD_ROOT.event_id!, undefined, [], false); await httpBackend.flushAllExpected(); const timelineSet = thread.timelineSet; - httpBackend - .when("GET", "/rooms/!foo%3Abar/event/" + encodeURIComponent(THREAD_ROOT.event_id!)) - .respond(200, function () { - return THREAD_ROOT; - }); - await flushHttp(emitPromise(thread, ThreadEvent.Update)); const timeline = await client.getEventTimeline(timelineSet, THREAD_REPLY.event_id!); @@ -1510,7 +1498,8 @@ describe("MatrixClient event timelines", function () { }, event: true, }); - THREAD_REPLY2.localTimestamp += 1000; + // this has to come after THREAD_REPLY which hasn't been instantiated by us + THREAD_REPLY2.localTimestamp += 10000000; // Test data for the first thread, with the second reply const THREAD_ROOT_UPDATED = { @@ -1570,9 +1559,6 @@ describe("MatrixClient event timelines", function () { thread.initialEventsFetched = true; const prom = emitPromise(room, ThreadEvent.NewReply); respondToEvent(THREAD_ROOT_UPDATED); - respondToEvent(THREAD_ROOT_UPDATED); - respondToEvent(THREAD_ROOT_UPDATED); - respondToEvent(THREAD_ROOT_UPDATED); respondToEvent(THREAD2_ROOT); await room.addLiveEvents([THREAD_REPLY2]); await httpBackend.flushAllExpected(); @@ -1699,13 +1685,11 @@ describe("MatrixClient event timelines", function () { thread.initialEventsFetched = true; const prom = emitPromise(room, ThreadEvent.Update); respondToEvent(THREAD_ROOT_UPDATED); - respondToEvent(THREAD_ROOT_UPDATED); - respondToEvent(THREAD_ROOT_UPDATED); respondToEvent(THREAD2_ROOT); await room.addLiveEvents([THREAD_REPLY_REACTION]); await httpBackend.flushAllExpected(); await prom; - expect(thread.length).toBe(2); + expect(thread.length).toBe(1); // reactions don't count towards the length of a thread // Test thread order is unchanged expect(timeline!.getEvents().map((it) => it.event.event_id)).toEqual([ THREAD_ROOT.event_id, @@ -2021,25 +2005,6 @@ describe("MatrixClient event timelines", function () { .respond(200, function () { return THREAD_ROOT; }); - httpBackend - .when("GET", "/rooms/!foo%3Abar/event/" + encodeURIComponent(THREAD_ROOT.event_id!)) - .respond(200, function () { - return THREAD_ROOT; - }); - httpBackend - .when( - "GET", - "/_matrix/client/v1/rooms/!foo%3Abar/relations/" + - encodeURIComponent(THREAD_ROOT.event_id!) + - "/" + - encodeURIComponent(THREAD_RELATION_TYPE.name) + - buildRelationPaginationQuery({ dir: Direction.Backward, limit: 1 }), - ) - .respond(200, function () { - return { - chunk: [THREAD_REPLY], - }; - }); await Promise.all([httpBackend.flushAllExpected(), utils.syncPromise(client)]); const room = client.getRoom(roomId)!; @@ -2047,71 +2012,7 @@ describe("MatrixClient event timelines", function () { expect(thread.initialEventsFetched).toBeTruthy(); const timelineSet = thread.timelineSet; - httpBackend - .when("GET", "/rooms/!foo%3Abar/event/" + encodeURIComponent(THREAD_ROOT.event_id!)) - .respond(200, function () { - return THREAD_ROOT; - }); - httpBackend - .when("GET", "/rooms/!foo%3Abar/event/" + encodeURIComponent(THREAD_ROOT.event_id!)) - .respond(200, function () { - return THREAD_ROOT; - }); - httpBackend - .when("GET", "/rooms/!foo%3Abar/event/" + encodeURIComponent(THREAD_ROOT.event_id!)) - .respond(200, function () { - return THREAD_ROOT; - }); - httpBackend - .when("GET", "/rooms/!foo%3Abar/event/" + encodeURIComponent(THREAD_ROOT.event_id!)) - .respond(200, function () { - return THREAD_ROOT; - }); - httpBackend - .when("GET", "/rooms/!foo%3Abar/context/" + encodeURIComponent(THREAD_ROOT.event_id!)) - .respond(200, function () { - return { - start: "start_token", - events_before: [], - event: THREAD_ROOT, - events_after: [], - end: "end_token", - state: [], - }; - }); - httpBackend - .when( - "GET", - "/_matrix/client/v1/rooms/!foo%3Abar/relations/" + - encodeURIComponent(THREAD_ROOT.event_id!) + - "/" + - encodeURIComponent(THREAD_RELATION_TYPE.name) + - buildRelationPaginationQuery({ - dir: Direction.Backward, - from: "start_token", - }), - ) - .respond(200, function () { - return { - chunk: [], - }; - }); - httpBackend - .when( - "GET", - "/_matrix/client/v1/rooms/!foo%3Abar/relations/" + - encodeURIComponent(THREAD_ROOT.event_id!) + - "/" + - encodeURIComponent(THREAD_RELATION_TYPE.name) + - buildRelationPaginationQuery({ dir: Direction.Forward, from: "end_token" }), - ) - .respond(200, function () { - return { - chunk: [THREAD_REPLY], - }; - }); - - const timeline = await flushHttp(client.getEventTimeline(timelineSet, THREAD_ROOT.event_id!)); + const timeline = await client.getEventTimeline(timelineSet, THREAD_ROOT.event_id!); httpBackend.when("GET", "/sync").respond(200, { next_batch: "s_5_5", diff --git a/spec/test-utils/thread.ts b/spec/test-utils/thread.ts index ebca5720fe4..bed1b235e87 100644 --- a/spec/test-utils/thread.ts +++ b/spec/test-utils/thread.ts @@ -157,7 +157,7 @@ export const mkThread = ({ room?.reEmitter.reEmit(evt, [MatrixEventEvent.BeforeRedaction]); } - const thread = room.createThread(rootEvent.getId() ?? "", rootEvent, events, true); + const thread = room.createThread(rootEvent.getId() ?? "", rootEvent, [rootEvent, ...events], true); return { thread, rootEvent, events }; }; diff --git a/spec/unit/models/thread.spec.ts b/spec/unit/models/thread.spec.ts index d8dd88809b4..0847bc8f054 100644 --- a/spec/unit/models/thread.spec.ts +++ b/spec/unit/models/thread.spec.ts @@ -18,7 +18,7 @@ import { mocked } from "jest-mock"; import { MatrixClient, PendingEventOrdering } from "../../../src/client"; import { Room, RoomEvent } from "../../../src/models/room"; -import { Thread, THREAD_RELATION_TYPE, ThreadEvent, FeatureSupport } from "../../../src/models/thread"; +import { FeatureSupport, Thread, THREAD_RELATION_TYPE, ThreadEvent } from "../../../src/models/thread"; import { makeThreadEvent, mkThread } from "../../test-utils/thread"; import { TestClient } from "../../TestClient"; import { emitPromise, mkEdit, mkMessage, mkReaction, mock } from "../../test-utils/test-utils"; @@ -43,6 +43,7 @@ describe("Thread", () => { const myUserId = "@bob:example.org"; const testClient = new TestClient(myUserId, "DEVICE", "ACCESS_TOKEN", undefined, { timelineSupport: false }); const client = testClient.client; + client.supportsThreads = jest.fn().mockReturnValue(true); const room = new Room("123", client, myUserId, { pendingEventOrdering: PendingEventOrdering.Detached, }); @@ -300,6 +301,7 @@ describe("Thread", () => { timelineSupport: false, }); const client = testClient.client; + client.supportsThreads = jest.fn().mockReturnValue(true); const room = new Room("123", client, myUserId, { pendingEventOrdering: PendingEventOrdering.Detached, }); @@ -354,6 +356,7 @@ describe("Thread", () => { timelineSupport: false, }); const client = testClient.client; + client.supportsThreads = jest.fn().mockReturnValue(true); const room = new Room("123", client, myUserId, { pendingEventOrdering: PendingEventOrdering.Detached, }); @@ -405,6 +408,7 @@ describe("Thread", () => { timelineSupport: false, }); const client = testClient.client; + client.supportsThreads = jest.fn().mockReturnValue(true); const room = new Room("123", client, myUserId, { pendingEventOrdering: PendingEventOrdering.Detached, }); @@ -699,11 +703,7 @@ async function createThread(client: MatrixClient, user: string, roomId: string): root.setThreadId(root.getId()); await room.addLiveEvents([root]); - // Create the thread and wait for it to be initialised - const thread = room.createThread(root.getId()!, root, [], false); - await new Promise((res) => thread.once(RoomEvent.TimelineReset, () => res())); - - return thread; + return room.createThread(root.getId()!, root, [], false); } /** diff --git a/spec/unit/room.spec.ts b/spec/unit/room.spec.ts index 150f2c30cb7..777d46c469f 100644 --- a/spec/unit/room.spec.ts +++ b/spec/unit/room.spec.ts @@ -2787,10 +2787,10 @@ describe("Room", function () { let prom = emitPromise(room, ThreadEvent.New); await room.addLiveEvents([threadRoot, threadResponse1]); const thread: Thread = await prom; - await emitPromise(room, ThreadEvent.Update); expect(thread.initialEventsFetched).toBeTruthy(); await room.addLiveEvents([threadResponse2]); + await emitPromise(room, ThreadEvent.Update); expect(thread).toHaveLength(2); expect(thread.replyToEvent!.getId()).toBe(threadResponse2.getId()); diff --git a/src/models/thread.ts b/src/models/thread.ts index 62d36ac7947..2966f22a545 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -98,6 +98,7 @@ export class Thread extends ReadReceipt; public initialEventsFetched = !Thread.hasServerSideSupport; /** @@ -134,6 +135,7 @@ export class Thread extends ReadReceipt => { + // We hit a gappy sync, ask the server for an update + await this.processRootEventPromise; + this.processRootEventPromise = undefined; + }; + private async fetchRootEvent(): Promise { this.rootEvent = this.room.findEventById(this.id); // If the rootEvent does not exist in the local stores, then fetch it from the server. @@ -197,6 +205,11 @@ export class Thread extends ReadReceipt { this.setEventMetadata(event); + if (!this.initialEventsFetched && !toStartOfTimeline && event.getId() === this.id) { + // We're loading the thread organically + this.initialEventsFetched = true; + } + const lastReply = this.lastReply(); const isNewestReply = !lastReply || event.localTimestamp >= lastReply!.localTimestamp; @@ -351,10 +374,14 @@ export class Thread extends ReadReceipt { - this.updatePendingReplyCount(); - + private async updateThreadFromRootEvent(): Promise { if (Thread.hasServerSideSupport) { // Ensure we show *something* as soon as possible, we'll update it as soon as we get better data, but we // don't want the thread preview to be empty if we can avoid it - if (!this.initialEventsFetched) { + if (!this.initialEventsFetched && !this.lastEvent) { await this.processRootEvent(); } await this.fetchRootEvent(); } await this.processRootEvent(); + } + + private async updateThreadMetadata(): Promise { + this.updatePendingReplyCount(); + + if (!this.processRootEventPromise) { + // We only want to do this once otherwise we end up rolling back to the last unsigned summary we have for the thread + this.processRootEventPromise = this.updateThreadFromRootEvent(); + } + await this.processRootEventPromise; if (!this.initialEventsFetched) { this.initialEventsFetched = true; @@ -572,7 +607,9 @@ export class Thread extends ReadReceipt boolean = (): boolean => true): MatrixEvent | null { + public lastReply( + matches: (ev: MatrixEvent) => boolean = (ev): boolean => ev.isRelation(RelationType.Thread), + ): MatrixEvent | null { for (let i = this.timeline.length - 1; i >= 0; i--) { const event = this.timeline[i]; if (matches(event)) {