Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix account & room settings race condition #7953

Merged
merged 5 commits into from
Mar 3, 2022
Merged

Fix account & room settings race condition #7953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3efd216
Fix Settings local echo caching bug
t3chguy Mar 2, 2022
be966ec
Fix async settings handlers wrongly upholding setValue interface promise
t3chguy Mar 3, 2022
78a0b31
Iterate PR
t3chguy Mar 3, 2022
2e95903
Update stubClient to stub the client in MatrixClientBackedSettingsHan…
t3chguy Mar 3, 2022
c0c9763
Merge branch 'develop' of github.com:matrix-org/matrix-react-sdk into…
t3chguy Mar 3, 2022
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
117 changes: 62 additions & 55 deletions src/settings/handlers/AccountSettingsHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ limitations under the License.

import { ClientEvent, MatrixClient } from "matrix-js-sdk/src/client";
import { MatrixEvent } from "matrix-js-sdk/src/models/event";
import { defer } from "matrix-js-sdk/src/utils";

import { MatrixClientPeg } from '../../MatrixClientPeg';
import MatrixClientBackedSettingsHandler from "./MatrixClientBackedSettingsHandler";
import { objectClone, objectKeyChanges } from "../../utils/objects";
import { SettingLevel } from "../SettingLevel";
Expand All @@ -30,6 +30,7 @@ const BREADCRUMBS_EVENT_TYPES = [BREADCRUMBS_LEGACY_EVENT_TYPE, BREADCRUMBS_EVEN
const RECENT_EMOJI_EVENT_TYPE = "io.element.recent_emoji";
const INTEG_PROVISIONING_EVENT_TYPE = "im.vector.setting.integration_provisioning";
const ANALYTICS_EVENT_TYPE = "im.vector.analytics";
const DEFAULT_SETTINGS_EVENT_TYPE = "im.vector.web.settings";

/**
* Gets and sets settings at the "account" level for the current user.
Expand All @@ -45,10 +46,7 @@ export default class AccountSettingsHandler extends MatrixClientBackedSettingsHa
}

public initMatrixClient(oldClient: MatrixClient, newClient: MatrixClient) {
if (oldClient) {
oldClient.removeListener(ClientEvent.AccountData, this.onAccountData);
}

oldClient?.removeListener(ClientEvent.AccountData, this.onAccountData);
newClient.on(ClientEvent.AccountData, this.onAccountData);
}

Expand All @@ -62,9 +60,9 @@ export default class AccountSettingsHandler extends MatrixClientBackedSettingsHa
}

this.watchers.notifyUpdate("urlPreviewsEnabled", null, SettingLevel.ACCOUNT, val);
} else if (event.getType() === "im.vector.web.settings" || event.getType() === ANALYTICS_EVENT_TYPE) {
} else if (event.getType() === DEFAULT_SETTINGS_EVENT_TYPE || event.getType() === ANALYTICS_EVENT_TYPE) {
// Figure out what changed and fire those updates
const prevContent = prevEvent ? prevEvent.getContent() : {};
const prevContent = prevEvent?.getContent() ?? {};
const changedSettings = objectKeyChanges<Record<string, any>>(prevContent, event.getContent());
for (const settingName of changedSettings) {
const val = event.getContent()[settingName];
Expand Down Expand Up @@ -136,72 +134,81 @@ export default class AccountSettingsHandler extends MatrixClientBackedSettingsHa
return preferredValue;
}

public async setValue(settingName: string, roomId: string, newValue: any): Promise<void> {
// Special case URL previews
if (settingName === "urlPreviewsEnabled") {
const content = this.getSettings("org.matrix.preview_urls") || {};
content['disable'] = !newValue;
await MatrixClientPeg.get().setAccountData("org.matrix.preview_urls", content);
return;
// helper function to set account data then await it being echoed back
private async setAccountData(
eventType: string,
field: string,
value: any,
legacyEventType?: string,
): Promise<void> {
let content = this.getSettings(eventType);
if (legacyEventType && !content?.[field]) {
content = this.getSettings(legacyEventType);
}

// Special case for breadcrumbs
if (settingName === "breadcrumb_rooms") {
// We read the value first just to make sure we preserve whatever random keys might be present.
let content = this.getSettings(BREADCRUMBS_EVENT_TYPE);
if (!content || !content['recent_rooms']) {
content = this.getSettings(BREADCRUMBS_LEGACY_EVENT_TYPE);
}
if (!content) content = {}; // If we still don't have content, make some

content['recent_rooms'] = newValue;
await MatrixClientPeg.get().setAccountData(BREADCRUMBS_EVENT_TYPE, content);
return;
if (!content) {
content = {};
}

// Special case recent emoji
if (settingName === "recent_emoji") {
const content = this.getSettings(RECENT_EMOJI_EVENT_TYPE) || {};
content["recent_emoji"] = newValue;
await MatrixClientPeg.get().setAccountData(RECENT_EMOJI_EVENT_TYPE, content);
return;
}
content[field] = value;

// Special case integration manager provisioning
if (settingName === "integrationProvisioning") {
const content = this.getSettings(INTEG_PROVISIONING_EVENT_TYPE) || {};
content['enabled'] = newValue;
await MatrixClientPeg.get().setAccountData(INTEG_PROVISIONING_EVENT_TYPE, content);
return;
}
await this.client.setAccountData(eventType, content);

// Special case analytics
if (settingName === "pseudonymousAnalyticsOptIn") {
const content = this.getSettings(ANALYTICS_EVENT_TYPE) || {};
content[settingName] = newValue;
await MatrixClientPeg.get().setAccountData(ANALYTICS_EVENT_TYPE, content);
return;
}
const deferred = defer<void>();
const handler = (event: MatrixEvent) => {
if (event.getType() !== eventType || event.getContent()[field] !== value) return;
this.client.off(ClientEvent.AccountData, handler);
deferred.resolve();
};
this.client.on(ClientEvent.AccountData, handler);

const content = this.getSettings() || {};
content[settingName] = newValue;
await MatrixClientPeg.get().setAccountData("im.vector.web.settings", content);
await deferred.promise;
}

public setValue(settingName: string, roomId: string, newValue: any): Promise<void> {
switch (settingName) {
// Special case URL previews
case "urlPreviewsEnabled":
return this.setAccountData("org.matrix.preview_urls", "disable", !newValue);

// Special case for breadcrumbs
case "breadcrumb_rooms":
return this.setAccountData(
BREADCRUMBS_EVENT_TYPE,
"recent_rooms",
newValue,
BREADCRUMBS_LEGACY_EVENT_TYPE,
);

// Special case recent emoji
case "recent_emoji":
return this.setAccountData(RECENT_EMOJI_EVENT_TYPE, "recent_emoji", newValue);

// Special case integration manager provisioning
case "integrationProvisioning":
return this.setAccountData(INTEG_PROVISIONING_EVENT_TYPE, "enabled", newValue);

// Special case analytics
case "pseudonymousAnalyticsOptIn":
return this.setAccountData(ANALYTICS_EVENT_TYPE, "pseudonymousAnalyticsOptIn", newValue);

default:
return this.setAccountData(DEFAULT_SETTINGS_EVENT_TYPE, settingName, newValue);
}
}

public canSetValue(settingName: string, roomId: string): boolean {
return true; // It's their account, so they should be able to
}

public isSupported(): boolean {
const cli = MatrixClientPeg.get();
return cli !== undefined && cli !== null && !cli.isGuest();
return this.client && !this.client.isGuest();
}

private getSettings(eventType = "im.vector.web.settings"): any { // TODO: [TS] Types on return
const cli = MatrixClientPeg.get();
if (!cli) return null;
if (!this.client) return null;

const event = cli.getAccountData(eventType);
const event = this.client.getAccountData(eventType);
if (!event || !event.getContent()) return null;
return objectClone(event.getContent()); // clone to prevent mutation
}
Expand Down
22 changes: 14 additions & 8 deletions src/settings/handlers/LocalEchoWrapper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,31 +40,37 @@ export default class LocalEchoWrapper extends SettingsHandler {
}

public getValue(settingName: string, roomId: string): any {
const cacheRoomId = roomId ? roomId : "UNDEFINED"; // avoid weird keys
const cacheRoomId = roomId ?? "UNDEFINED"; // avoid weird keys
const bySetting = this.cache[settingName];
if (bySetting && bySetting.hasOwnProperty(cacheRoomId)) {
if (bySetting?.hasOwnProperty(cacheRoomId)) {
return bySetting[cacheRoomId];
}

return this.handler.getValue(settingName, roomId);
}

public setValue(settingName: string, roomId: string, newValue: any): Promise<void> {
public async setValue(settingName: string, roomId: string, newValue: any): Promise<void> {
if (!this.cache[settingName]) this.cache[settingName] = {};
const bySetting = this.cache[settingName];

const cacheRoomId = roomId ? roomId : "UNDEFINED"; // avoid weird keys
const cacheRoomId = roomId ?? "UNDEFINED"; // avoid weird keys
bySetting[cacheRoomId] = newValue;

const currentValue = this.handler.getValue(settingName, roomId);
const handlerPromise = this.handler.setValue(settingName, roomId, newValue);
this.handler.watchers?.notifyUpdate(settingName, roomId, this.level, newValue);
return Promise.resolve(handlerPromise).catch(() => {

try {
await handlerPromise;
} catch (e) {
// notify of a rollback
this.handler.watchers?.notifyUpdate(settingName, roomId, this.level, currentValue);
}).finally(() => {
delete bySetting[cacheRoomId];
});
} finally {
// only expire the cache if our value hasn't been overwritten yet
if (bySetting[cacheRoomId] === newValue) {
delete bySetting[cacheRoomId];
}
}
}

public canSetValue(settingName: string, roomId: string): boolean {
Expand Down
5 changes: 1 addition & 4 deletions src/settings/handlers/MatrixClientBackedSettingsHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ limitations under the License.
*/

import { MatrixClient } from "matrix-js-sdk/src/client";
import { logger } from "matrix-js-sdk/src/logger";

import SettingsHandler from "./SettingsHandler";

Expand Down Expand Up @@ -49,7 +48,5 @@ export default abstract class MatrixClientBackedSettingsHandler extends Settings
return MatrixClientBackedSettingsHandler._matrixClient;
}

protected initMatrixClient(oldClient: MatrixClient, newClient: MatrixClient) {
logger.warn("initMatrixClient not overridden");
}
protected abstract initMatrixClient(oldClient: MatrixClient, newClient: MatrixClient);
}
74 changes: 47 additions & 27 deletions src/settings/handlers/RoomAccountSettingsHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,15 @@ limitations under the License.
import { MatrixClient } from "matrix-js-sdk/src/client";
import { MatrixEvent } from "matrix-js-sdk/src/models/event";
import { Room, RoomEvent } from "matrix-js-sdk/src/models/room";
import { defer } from "matrix-js-sdk/src/utils";

import { MatrixClientPeg } from '../../MatrixClientPeg';
import MatrixClientBackedSettingsHandler from "./MatrixClientBackedSettingsHandler";
import { objectClone, objectKeyChanges } from "../../utils/objects";
import { SettingLevel } from "../SettingLevel";
import { WatchManager } from "../WatchManager";

const ALLOWED_WIDGETS_EVENT_TYPE = "im.vector.setting.allowed_widgets";
const DEFAULT_SETTINGS_EVENT_TYPE = "im.vector.web.settings";

/**
* Gets and sets settings at the "room-account" level for the current user.
Expand Down Expand Up @@ -55,7 +56,7 @@ export default class RoomAccountSettingsHandler extends MatrixClientBackedSettin
}

this.watchers.notifyUpdate("urlPreviewsEnabled", roomId, SettingLevel.ROOM_ACCOUNT, val);
} else if (event.getType() === "im.vector.web.settings") {
} else if (event.getType() === DEFAULT_SETTINGS_EVENT_TYPE) {
// Figure out what changed and fire those updates
const prevContent = prevEvent ? prevEvent.getContent() : {};
const changedSettings = objectKeyChanges<Record<string, any>>(prevContent, event.getContent());
Expand Down Expand Up @@ -87,43 +88,62 @@ export default class RoomAccountSettingsHandler extends MatrixClientBackedSettin
return settings[settingName];
}

public async setValue(settingName: string, roomId: string, newValue: any): Promise<void> {
// Special case URL previews
if (settingName === "urlPreviewsEnabled") {
const content = this.getSettings(roomId, "org.matrix.room.preview_urls") || {};
content['disable'] = !newValue;
await MatrixClientPeg.get().setRoomAccountData(roomId, "org.matrix.room.preview_urls", content);
return;
// helper function to send room account data then await it being echoed back
private async setRoomAccountData(
roomId: string,
eventType: string,
field: string | null,
value: any,
): Promise<void> {
let content: ReturnType<RoomAccountSettingsHandler["getSettings"]>;

if (field === null) {
content = value;
} else {
const content = this.getSettings(roomId, eventType) || {};
content[field] = value;
}

// Special case allowed widgets
if (settingName === "allowedWidgets") {
await MatrixClientPeg.get().setRoomAccountData(roomId, ALLOWED_WIDGETS_EVENT_TYPE, newValue);
return;
}
await this.client.setRoomAccountData(roomId, eventType, content);

const deferred = defer<void>();
const handler = (event: MatrixEvent) => {
if (event.getRoomId() !== roomId || event.getType() !== eventType) return;
if (field !== null && event.getContent()[field] !== value) return;
this.client.off(RoomEvent.AccountData, handler);
deferred.resolve();
};
this.client.on(RoomEvent.AccountData, handler);

const content = this.getSettings(roomId) || {};
content[settingName] = newValue;
await MatrixClientPeg.get().setRoomAccountData(roomId, "im.vector.web.settings", content);
await deferred.promise;
}

public canSetValue(settingName: string, roomId: string): boolean {
const room = MatrixClientPeg.get().getRoom(roomId);
public setValue(settingName: string, roomId: string, newValue: any): Promise<void> {
switch (settingName) {
// Special case URL previews
case "urlPreviewsEnabled":
return this.setRoomAccountData(roomId, "org.matrix.room.preview_urls", "disable", !newValue);

// Special case allowed widgets
case "allowedWidgets":
return this.setRoomAccountData(roomId, ALLOWED_WIDGETS_EVENT_TYPE, null, newValue);

default:
return this.setRoomAccountData(roomId, DEFAULT_SETTINGS_EVENT_TYPE, settingName, newValue);
}
}

public canSetValue(settingName: string, roomId: string): boolean {
// If they have the room, they can set their own account data
return room !== undefined && room !== null;
return !!this.client.getRoom(roomId);
}

public isSupported(): boolean {
const cli = MatrixClientPeg.get();
return cli !== undefined && cli !== null && !cli.isGuest();
return this.client && !this.client.isGuest();
}

private getSettings(roomId: string, eventType = "im.vector.web.settings"): any { // TODO: [TS] Type return
const room = MatrixClientPeg.get().getRoom(roomId);
if (!room) return null;

const event = room.getAccountData(eventType);
private getSettings(roomId: string, eventType = DEFAULT_SETTINGS_EVENT_TYPE): any { // TODO: [TS] Type return
const event = this.client.getRoom(roomId)?.getAccountData(eventType);
if (!event || !event.getContent()) return null;
return objectClone(event.getContent()); // clone to prevent mutation
}
Expand Down
Loading