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

Reduce our reliance on an up-to-date joined rooms cache #16

Merged
merged 6 commits into from
Jun 5, 2023
Merged

Reduce our reliance on an up-to-date joined rooms cache #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fadc139
Reduce our reliance on an up-to-date joined rooms cache
tadzik May 5, 2023
98b3d46
Remove joined room caching entirely
tadzik May 26, 2023
f6df91f
Merge branch 'main' into tadzik/safer-joined-rooms-caching
tadzik May 26, 2023
728cf8e
Remove now-unused variables
tadzik May 26, 2023
f183877
Undo accidental API breakage in getJoinedRooms()
tadzik Jun 5, 2023
b2a2bc1
Deprecate ensureJoined()
tadzik Jun 5, 2023
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
2 changes: 0 additions & 2 deletions src/appservice/Appservice.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,9 +631,7 @@ export class Appservice extends EventEmitter {
const botDomain = new UserID(this.botUserId).domain;
if (domain !== botDomain) return; // can't be impersonated, so don't try

// Update the target intent's joined rooms (fixes transition errors with the cache, like join->kick->join)
const intent = this.getIntentForUserId(event['state_key']);
await intent.refreshJoinedRooms();

const targetMembership = event["content"]["membership"];
if (targetMembership === "join") {
Expand Down
39 changes: 8 additions & 31 deletions src/appservice/Intent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ export class Intent {

private client: MatrixClient;
private unstableApisInstance: UnstableAppserviceApis;
private knownJoinedRooms: string[] = [];
private cryptoSetupPromise: Promise<void>;

/**
Expand Down Expand Up @@ -169,10 +168,9 @@ export class Intent {
}

// Now set up crypto
await this.client.crypto.prepare(await this.refreshJoinedRooms());
await this.client.crypto.prepare(await this.getJoinedRooms());

this.appservice.on("room.event", (roomId, event) => {
if (!this.knownJoinedRooms.includes(roomId)) return;
this.client.crypto.onRoomEvent(roomId, event);
});

Expand All @@ -186,16 +184,14 @@ export class Intent {
}

/**
* Gets the joined rooms for the intent. Note that by working around
* the intent to join rooms may yield inaccurate results.
* Gets the joined rooms for the intent.
* @returns {Promise<string[]>} Resolves to an array of room IDs where
* the intent is joined.
*/
@timedIntentFunctionCall()
public async getJoinedRooms(): Promise<string[]> {
await this.ensureRegistered();
if (this.knownJoinedRooms.length === 0) await this.refreshJoinedRooms();
return this.knownJoinedRooms.map(r => r); // clone
return await this.client.getJoinedRooms();
}

/**
Expand All @@ -207,10 +203,7 @@ export class Intent {
@timedIntentFunctionCall()
public async leaveRoom(roomId: string, reason?: string): Promise<any> {
await this.ensureRegistered();
return this.client.leaveRoom(roomId, reason).then(async () => {
// Recalculate joined rooms now that we've left a room
await this.refreshJoinedRooms();
});
return this.client.leaveRoom(roomId, reason);
}

/**
Expand All @@ -221,11 +214,7 @@ export class Intent {
@timedIntentFunctionCall()
public async joinRoom(roomIdOrAlias: string): Promise<string> {
await this.ensureRegistered();
return this.client.joinRoom(roomIdOrAlias).then(async roomId => {
// Recalculate joined rooms now that we've joined a room
await this.refreshJoinedRooms();
return roomId;
});
return this.client.joinRoom(roomIdOrAlias);
}

/**
Expand Down Expand Up @@ -267,35 +256,23 @@ export class Intent {
* Ensures the user is joined to the given room
* @param {string} roomId The room ID to join
* @returns {Promise<any>} Resolves when complete
* @deprecated Use `joinRoom()` instead
*/
@timedIntentFunctionCall()
public async ensureJoined(roomId: string) {
if (this.knownJoinedRooms.indexOf(roomId) !== -1) {
return;
}

await this.refreshJoinedRooms();

if (this.knownJoinedRooms.indexOf(roomId) !== -1) {
return;
}

const returnedRoomId = await this.client.joinRoom(roomId);
if (!this.knownJoinedRooms.includes(returnedRoomId)) {
this.knownJoinedRooms.push(returnedRoomId);
}
return returnedRoomId;
}

/**
* Refreshes which rooms the user is joined to, potentially saving time on
* calls like ensureJoined()
* @deprecated There is no longer a joined rooms cache, use `getJoinedRooms()` instead
* @returns {Promise<string[]>} Resolves to the joined room IDs for the user.
*/
@timedIntentFunctionCall()
public async refreshJoinedRooms(): Promise<string[]> {
this.knownJoinedRooms = await this.client.getJoinedRooms();
return this.knownJoinedRooms.map(r => r); // clone
return await this.getJoinedRooms();
}

/**
Expand Down
94 changes: 1 addition & 93 deletions test/appservice/AppserviceTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1752,7 +1752,7 @@ describe('Appservice', () => {
};

const intent = appservice.getIntentForSuffix("test");
intent.refreshJoinedRooms = () => Promise.resolve([]);
intent.getJoinedRooms = () => Promise.resolve([]);

await appservice.begin();

Expand Down Expand Up @@ -1857,98 +1857,6 @@ describe('Appservice', () => {
}
});

it('should refresh membership information of intents when actions are performed against them', async () => {
const port = await getPort();
const hsToken = "s3cret_token";
const appservice = new Appservice({
port: port,
bindAddress: '',
homeserverName: 'example.org',
homeserverUrl: 'https://localhost',
registration: {
as_token: "",
hs_token: hsToken,
sender_localpart: "_bot_",
namespaces: {
users: [{ exclusive: true, regex: "@_prefix_.*:.+" }],
rooms: [],
aliases: [],
},
},
});
appservice.botIntent.ensureRegistered = () => {
return null;
};

await appservice.begin();

try {
const intent = appservice.getIntentForSuffix("test");
const refreshSpy = simple.stub().callFn(() => Promise.resolve([]));
intent.refreshJoinedRooms = refreshSpy;

// polyfill the dummy user too
const intent2 = appservice.getIntentForSuffix("test___WRONGUSER");
intent2.refreshJoinedRooms = () => Promise.resolve([]);

const joinTxn = {
events: [
{
type: "m.room.member",
room_id: "!AAA:example.org",
content: { membership: "join" },
state_key: "@_prefix_test:example.org",
sender: "@_prefix_test:example.org",
},
{
type: "m.room.member",
room_id: "!AAA:example.org",
content: { membership: "join" },
state_key: "@_prefix_test___WRONGUSER:example.org",
sender: "@_prefix_test:example.org",
},
],
};
const kickTxn = {
events: [
{
type: "m.room.member",
room_id: "!AAA:example.org",
content: { membership: "leave" },
state_key: "@_prefix_test:example.org",
sender: "@someone_else:example.org",
},
{
type: "m.room.member",
room_id: "!AAA:example.org",
content: { membership: "leave" },
state_key: "@_prefix_test___WRONGUSER:example.org",
sender: "@someone_else:example.org",
},
],
};

// eslint-disable-next-line no-inner-declarations
async function doCall(route: string, opts: any = {}) {
const res = await requestPromise({
uri: `http://localhost:${port}${route}`,
method: "PUT",
qs: { access_token: hsToken },
...opts,
});
expect(res).toMatchObject({});

expect(refreshSpy.callCount).toBe(1);
refreshSpy.callCount = 0;
}

await doCall("/transactions/1", { json: joinTxn });
await doCall("/_matrix/app/v1/transactions/2", { json: kickTxn });
} finally {
appservice.stop();
}
});

it('should handle room upgrade events in transactions', async () => {
const port = await getPort();
const hsToken = "s3cret_token";
Expand Down
Loading